Re: [PATCH 1/3] drm/xe/ggtt: stop mapping unmapped GGTT pages to scratch

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Aug 24, 2026 at 11:17:57AM -0700, Niranjana Vishwanathapura wrote:
> On Mon, Aug 24, 2026 at 10:04:56PM +0530, tilak.tirumalesh.tangudu@intel.com wrote:
> > From: Tangudu Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>
> > 
> > xe_ggtt_clear() wrote a scratch PTE across the range for both
> > initial_clear() and the node_remove() unmap path.
> > 
> > On unmap the scratch PTE hides stale-TLB gaps: after the range is
> > invalidated a re-walk lands on the still-valid scratch PTE, so a stale
> > engine TLB translation for the freed range never faults and the
> > invalidation gap stays invisible.
> > 
> > Add an explicit use_scratch bool. initial_clear() keeps writing scratch.
> > On multi-queue platforms, where engines cache GGTT independently,
> > node_remove() writes PTE=0 so a stale translation to a freed range
> > faults and the gap becomes observable; other platforms keep scratch.
> > 
> > v2: Add xe_gt_has_multi_queue() helper and other knits-Niranjana
> > 

I'm a little confused about the rationale here. Can you elaborate on why
this is being done?

I also don't really like that we boot with the entire GGTT pointing to
scratch pages and then, over time, as we map and unmap entries in the
GGTT, it gradually becomes populated with unmapped entries that no
longer point to scratch.

This means the behavior can change over time. For example, immediately
after boot, prefetches into unmapped GGTT regions do not fault because
they hit scratch pages. Later on, the same prefetch may fault because
the corresponding GGTT entry no longer points to scratch.

The answer for scratch needs to be either "on" or "off", not something
that changes dynamically at unmap time. I'm fine with deciding to turn
this off, but the behavior needs to remain consistent for a given boot
and platform.

Matt

> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Tangudu Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_ggtt.c | 47 ++++++++++++++++++++++++++----------
> > drivers/gpu/drm/xe/xe_gt.h   | 13 ++++++++++
> > 2 files changed, 47 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> > index 8ec23862477f..4cddd69f0e99 100644
> > --- a/drivers/gpu/drm/xe/xe_ggtt.c
> > +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> > @@ -20,6 +20,7 @@
> > #include "regs/xe_regs.h"
> > #include "xe_assert.h"
> > #include "xe_bo.h"
> > +#include "xe_gt.h"
> > #include "xe_gt_printk.h"
> > #include "xe_gt_types.h"
> > #include "xe_map.h"
> > @@ -257,23 +258,36 @@ static u64 xe_ggtt_get_pte(struct xe_ggtt *ggtt, u64 addr)
> > 	return readq(&ggtt->gsm[addr >> XE_PTE_SHIFT]);
> > }
> > 
> > -static void xe_ggtt_clear(struct xe_ggtt *ggtt, u64 start, u64 size)
> > +/**
> > + * xe_ggtt_clear() - Clear a range of GGTT page-table entries
> > + * @ggtt: the &xe_ggtt to clear
> > + * @start: start of the range (GGTT address)
> > + * @size: size of the range in bytes
> > + * @use_scratch: true fills the range with the scratch PTE (init-time clear so
> > + *		 engines walking free holes read zeros safely); false writes
> > + *		 PTE=0 so a stale HW TLB entry faults instead of silently
> > + *		 landing on scratch.
> > + */
> 
> NIT...Kernel-doc not really needed for static functions. General comment section
> should be good. I prefer keeping it simple with general comment, but leaving it
> as is is fine too.
> 
> In any case,
> Reviewed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> 
> > +static void xe_ggtt_clear(struct xe_ggtt *ggtt, u64 start, u64 size,
> > +			  bool use_scratch)
> > {
> > -	u16 pat_index = xe_cache_pat_idx(tile_to_xe(ggtt->tile), XE_CACHE_WB);
> > 	u64 end = start + size - 1;
> > -	u64 scratch_pte;
> > +	u64 pte;
> > 
> > 	xe_tile_assert(ggtt->tile, start < end);
> > 
> > -	if (ggtt->scratch)
> > -		scratch_pte = xe_bo_addr(ggtt->scratch, 0, XE_PAGE_SIZE) |
> > -			      ggtt->pt_ops->pte_encode_flags(ggtt->scratch,
> > -							     pat_index);
> > -	else
> > -		scratch_pte = 0;
> > +	if (use_scratch && ggtt->scratch) {
> > +		u16 pat_index = xe_cache_pat_idx(tile_to_xe(ggtt->tile),
> > +						 XE_CACHE_WB);
> > +
> > +		pte = xe_bo_addr(ggtt->scratch, 0, XE_PAGE_SIZE) |
> > +		      ggtt->pt_ops->pte_encode_flags(ggtt->scratch, pat_index);
> > +	} else {
> > +		pte = 0;
> > +	}
> > 
> > 	while (start < end) {
> > -		ggtt->pt_ops->ggtt_set_pte(ggtt, start, scratch_pte);
> > +		ggtt->pt_ops->ggtt_set_pte(ggtt, start, pte);
> > 		start += XE_PAGE_SIZE;
> > 	}
> > }
> > @@ -459,7 +473,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->start + start, end - start, true);
> > 
> > 	xe_ggtt_invalidate(ggtt);
> > 	mutex_unlock(&ggtt->lock);
> > @@ -473,12 +487,19 @@ static void ggtt_node_fini(struct xe_ggtt_node *node)
> > static void ggtt_node_remove(struct xe_ggtt_node *node)
> > {
> > 	struct xe_ggtt *ggtt = node->ggtt;
> > -	bool bound;
> > +	bool use_scratch, bound;
> > +
> > +	/*
> > +	 * Addition of a secondary queue while a multi-queue context is live on
> > +	 * HW requires proper GGTT TLB clearing, so use faulting PTEs here.
> > +	 */
> > +	use_scratch = !xe_gt_has_multi_queue(ggtt->tile->primary_gt);
> > 
> > 	mutex_lock(&ggtt->lock);
> > 	bound = ggtt->flags & XE_GGTT_FLAGS_ONLINE;
> > 	if (bound)
> > -		xe_ggtt_clear(ggtt, xe_ggtt_node_addr(node), xe_ggtt_node_size(node));
> > +		xe_ggtt_clear(ggtt, xe_ggtt_node_addr(node),
> > +			      xe_ggtt_node_size(node), use_scratch);
> > 	drm_mm_remove_node(&node->base);
> > 	node->base.size = 0;
> > 	if (bound && node->invalidate_on_remove)
> > diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
> > index 65a4655b0994..92d934d50244 100644
> > --- a/drivers/gpu/drm/xe/xe_gt.h
> > +++ b/drivers/gpu/drm/xe/xe_gt.h
> > @@ -160,4 +160,17 @@ static inline bool xe_gt_supports_multi_queue(const struct xe_gt *gt,
> > 	return gt->info.multi_queue_engine_class_mask & BIT(class);
> > }
> > 
> > +/**
> > + * xe_gt_has_multi_queue() - Check if gt supports multi-queue on
> > + * any engine class.
> > + *
> > + * @gt: the GT object (may be NULL)
> > + *
> > + * Return: true if any engine class on @gt supports multi queue, else false
> > + */
> > +static inline bool xe_gt_has_multi_queue(const struct xe_gt *gt)
> > +{
> > +	return gt && gt->info.multi_queue_engine_class_mask;
> > +}
> > +
> > #endif
> > -- 
> > 2.46.0
> >