Re: [PATCH] drm/xe/bo: Take a runtime PM ref when shrinking a bo needing invalidation

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi!


On Thu, 2026-08-27 at 09:44 -0700, Matthew Brost wrote:
> On Thu, Aug 27, 2026 at 10:20:10AM -0600, Lin, Shuicheng wrote:
> > On Wed, Aug 26, 2026 10:49 PM Matthew Brost wrote:
> > > On Wed, Aug 26, 2026 at 10:10:38PM +0000, Lin, Shuicheng wrote:
> > > > On Wed, Aug 26, 2026 1:01 AM Thomas Hellström wrote:
> > > > > On Tue, 2026-08-25 at 22:48 +0000, Shuicheng Lin wrote:
> > > > > > xe_bo_shrink() only took a runtime PM reference for the
> > > > > > System CCS
> > > > > > backup case, and only after the purgeable branch had
> > > > > > already returned.
> > > > > > That branch calls xe_bo_move_notify(), which reaches
> > > > > > xe_bo_trigger_rebind() -> xe_vm_invalidate_vma() and
> > > > > > submits a TLB
> > > > > > invalidation over GuC CT.  The non-purgeable branch reaches
> > > > > > the
> > > > > > same code through ttm_bo_shrink(.allow_move = true) ->
> > > > > > xe_bo_move().
> > > > > > 
> > > > > > When the device is runtime suspended the CT is disabled and
> > > > > > the
> > > > > > send returns -ENODEV, tripping the XE_WARN_ON() in
> > > xe_bo_trigger_rebind():
> > > > > > 
> > > > > >   WARNING: drivers/gpu/drm/xe/xe_bo.c:770 at
> > > > > > xe_bo_move_notify+0x1fc/0x450 [xe], CPU#2: xe_madvise/21389
> > > > > >    xe_bo_shrink+0x20f/0x2b0 [xe]
> > > > > >    __xe_shrinker_walk+0x174/0x410 [xe]
> > > > > >    xe_shrinker_walk+0x56/0xf0 [xe]
> > > > > >    xe_shrinker_scan+0x10c/0x1e0 [xe]
> > > > > >    do_shrink_slab+0x176/0x7e0
> > > > > >    shrink_slab+0x137/0x990
> > > > > >    drop_slab+0x7f/0x130
> > > > > >    drop_caches_sysctl_handler+0x9c/0xf0
> > > > > > 
> > > > > > The invalidation is always issued for a fault-mode vm;
> > > > > > since
> > > > > > commit 4e7ebff69aed ("drm/xe/xe3p_lpg: flush shrinker bo
> > > > > > cachelines
> > > > > > manually")
> > > > > > it is also issued for a non-fault-mode vm on hardware with
> > > > > > an
> > > > > > optimized
> > > > > > L2 flush, which is how this surfaced.
> > > > > > 
> > > > > > Add bo_needs_invalidate(), mirroring that condition, and
> > > > > > use it in
> > > > > > xe_bo_shrink() to compute needs_rpm ahead of both
> > > > > > branches.  The
> > > > > > reference is then held across xe_bo_move_notify() in either
> > > > > > path,
> > > > > > and is not taken for a bo whose mappings would not have
> > > > > > been invalidated.
> > > > > > 
> > > > > > Shrinking can run in reclaim contexts where the device may
> > > > > > not be
> > > > > > resumed, so a bo is skipped when the reference cannot be
> > > > > > acquired.
> > > > > > Have
> > > > > > that skip queue the shrinker PM worker:
> > > > > > xe_shrinker_runtime_pm_get() is gated on the needs of its
> > > > > > own
> > > > > > backup pass rather than on this one, and on DGFX it returns
> > > > > > before
> > > > > > queueing anything, so without this a scan where every
> > > > > > candidate is
> > > > > > skipped makes no progress, reports nothing scanned and
> > > > > > returns
> > > SHRINK_STOP with nothing arranging a wake.
> > > > > > 
> > > > > > Also gate the System CCS term on !xe_tt->purgeable, since
> > > > > > xe_bo_shrink_purge() frees the pages without a GPU copy.
> > > > > > 
> > > > > > Reproduced with igt@xe_madvise@dontneed-before-exec while
> > > > > > the GPU
> > > > > > is runtime suspended.
> > > > > > 
> > > > > > Fixes: 00c8efc3180f ("drm/xe: Add a shrinker for xe bos")
> > > > > > Assisted-by: Claude:claude-opus-5
> > > > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > > > > Signed-off-by: Shuicheng Lin <shuicheng.lin@intel.com>
> > > > > 
> > > > > Nice catch.
> > > > > 
> > > > > I wonder, however, can we skip the TLB flush if runtime PM is
> > > > > not
> > > > > available at TLB flush time (runtime_pm_get_if_active()?)
> > > > > Assuming
> > > > > that if runtime PM is not available, no contexts can be
> > > > > active and
> > > > > they will flush TLB implicitly when becoming active?
> > > > 
> > > > Do you mean call runtime_pm_get_if_active() in
> > > > xe_tlb_inval_range_tilemask_submit(),
> > > > or lower down in xe_tlb_inval_fence_init()?
> > > > I'd prefer that too, as it would simplify the code. Two things
> > > > stop me doing it
> > > here.
> > > > 
> > > > First, the PTE zap also needs the device, and it runs before
> > > > the flush.
> > > > xe_pt_zap_ptes_entry() does xe_map_memset(), which asserts via
> > > > xe_device_assert_mem_access():
> > > > 
> > > 
> > > I agree with this - the zap is not optional for fault mode, it
> > > probably isn't for
> > > 'xe_device_is_l2_flush_optimized'.
> > > 
> > > >   Assertion `!xe_pm_runtime_suspended(xe)` failed!
> > > >    xe_pt_zap_ptes_entry+0xb8/0x120 [xe]
> > > >    xe_vm_invalidate_vma_submit+0xb1/0x770 [xe]
> > > >    xe_bo_move_notify+0x1b3/0x450 [xe]
> > > >    xe_bo_shrink+0x20f/0x2b0 [xe]
> > > > 
> > > > The zap isn't optional - the pages are being freed, so the PTEs
> > > > have
> > > > to be cleared either way - and xe_pt_create() uses
> > > > XE_BO_FLAG_VRAM_IF_DGFX(), so on discrete it is a BAR write
> > > > that needs
> > > > D0. Skipping the flush alone leaves that behind.
> > > > 
> > > > Second, I can't find the implicit flush in the code.
> > > > xe_pm_runtime_resume() only calls xe_gt_resume()
> > > > (do_gt_restart())
> > > > when d3cold.allowed; otherwise xe_gt_runtime_resume() just
> > > > takes
> > > > forcewake and does xe_uc_runtime_resume() - no reset, no
> > > > invalidation.
> > > > That doesn't mean the TLBs survive, it may well be a property
> > > > of the
> > > > power state, but I can't confirm the assumption by reading the
> > > > driver.
> > > > Do you know if that is guaranteed? Thanks.
> > > > 
> > > > > 
> > > > > IIRC It's not totally clear whether this would work for
> > > > > "has_ctx_tlb_inval"
> > > > > hardware, and if so we might need to add something similar to
> > > > > this
> > > > > for that hardware.
> > > > 
> > > > My understanding is that it falls out for free if the check
> > > > sits above
> > > xe_tlb_inval_issue().
> > > > Is it right? Thanks.
> > > > 
> > > > Shuicheng
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > Thomas
> > > > > 
> > > > > > ---
> > > > > >  drivers/gpu/drm/xe/xe_bo.c       | 62
> > > > > > +++++++++++++++++++++++++++---
> > > > > > --
> > > > > >  drivers/gpu/drm/xe/xe_shrinker.c | 18 +++++++++-
> > > > > >  drivers/gpu/drm/xe/xe_shrinker.h |  2 ++
> > > > > >  3 files changed, 72 insertions(+), 10 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/xe/xe_bo.c
> > > > > > b/drivers/gpu/drm/xe/xe_bo.c index
> > > > > > 2eb5d6aac523..274ff97d8542
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/xe/xe_bo.c
> > > > > > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > > > > > @@ -734,6 +734,32 @@ static int
> > > > > > xe_ttm_io_mem_reserve(struct
> > > > > > ttm_device *bdev,
> > > > > >  	}
> > > > > >  }
> > > > > > 
> > > > > > +/*
> > > > > > + * Whether xe_bo_move_notify() will invalidate the GPU
> > > > > > mappings
> > > > > > +of
> > > > > > @bo, and
> > > > > > + * therefore needs the device resumed to reach the GuC.
> > > > > > + *
> > > > > > + * This mirrors the condition under which
> > > > > > xe_bo_trigger_rebind()
> > > > > > below calls
> > > > > > + * xe_vm_invalidate_vma(): always for a fault-mode vm, and
> > > > > > for
> > > > > > + any
> > > > > > bound vm on
> > > > > > + * hardware where the L2 flush is optimized. Keep the two
> > > > > > in sync.
> > > > > > + *
> > > > > > + * Context: Caller must hold the BO's dma-resv lock.
> > > > > > + */
> > > > > > +static bool bo_needs_invalidate(struct xe_bo *bo) {
> > > > > > +	struct drm_gpuvm_bo *vm_bo;
> > > > > > +
> > > > > > +	if
> > > > > > (xe_device_is_l2_flush_optimized(xe_bo_device(bo)))
> > > > > > +		return xe_bo_is_vm_bound(bo);
> > > > > > +
> > > > > > +	xe_bo_assert_held(bo);
> > > > > > +
> > > > > > +	drm_gem_for_each_gpuvm_bo(vm_bo, &bo->ttm.base)
> > > > > > +		if (xe_vm_in_fault_mode(gpuvm_to_vm(vm_bo-
> > > > > > >vm)))
> > > > > > +			return true;
> > > > > > +
> > > > > > +	return false;
> > > > > > +}
> > > > > > +
> > > > > >  static int xe_bo_trigger_rebind(struct xe_device *xe,
> > > > > > struct
> > > > > > xe_bo *bo,
> > > > > >  				const struct
> > > > > > ttm_operation_ctx *ctx)
> > > > > >  {
> > > > > > @@ -1361,6 +1387,28 @@ long xe_bo_shrink(struct
> > > > > > ttm_operation_ctx
> > > > > > *ctx, struct ttm_buffer_object *bo,
> > > > > >  	if (!xe_bo_is_xe_bo(bo) ||
> > > > > > !xe_bo_get_unless_zero(xe_bo))
> > > > > >  		return xe_bo_shrink_purge(ctx, bo,
> > > > > > scanned);
> > > > > > 
> > > > > > +	/*
> > > > > > +	 * Moving this bo out of a non-system placement
> > > > > > makes
> > > > > > +	 * xe_bo_move_notify() invalidate its GPU mappings
> > > > > > over GuC
> > > > > > CT, and
> > > > > > +	 * System CCS needs a gpu copy when moving PL_TT -
> > > > > > >
> > > > > > PL_SYSTEM. Both
> > > > > > +	 * need the device resumed.
> > > > > > +	 */
> > > > > > +	needs_rpm = bo->resource->mem_type != XE_PL_SYSTEM
> > > > > > &&
> > > > > > +		(bo_needs_invalidate(xe_bo) ||
> > > > > > +		 (!xe_tt->purgeable && !IS_DGFX(xe) &&
> > > > > > +		  xe_bo_needs_ccs_pages(xe_bo)));
> > > 
> > > This is a complex enough conditional that my current feeling is
> > > to just
> > > unconditionally set:
> > > 
> > > needs_rpm = true;
> > 
> > How about keep the XE_PL_SYSTEM check?
> > 
> 
> I think checking system is reasonable, and better than my suggestion,
> as
> this implies we don't have GPU pages (no invalidation) and no need
> for a
> CCS copy, and is keep check which is easy to understand.
> 
> > 	/* Both the invalidation and the System CCS copy need the
> > device. */
> > 	needs_rpm = bo->resource->mem_type != XE_PL_SYSTEM;
> > 
> > @Thomas Hellström you wrote the original code, what do you think?
> > 
> 
> Yes, let's see what Thomas says.

I think keeping the system check makes sense, and for now a quick fix.

However I think avoiding waking the device for small reclaims makes
sense, so we should also look at the shrink batch series trying to make
that merged.

If we wanted to optimize this, and add back the complexity, I think
calling xe_vm_invalidate_vma() would only make sense if runtime_pm is
available:

1) We need to do it on faulting VMs -> Runtime PM always available.
2) We need to do it to trap L1 cache flushes and on l2_flush_optimized
to flush out l2 before reclaiming -> Can we assume that the device
always does this before going to sleep? Need to confirm that with
Ankur.

Thanks,
Thomas


> 
> Mattb
> 
> > > 
> > > In practice, on iGPUs where the display is in use, we always hold
> > > a runtime PM
> > > reference. It would be very unusual for the display to be
> > > suspended while the
> > > shrinker is running, at least as far as I can tell.
> > > Let's drop this hard-to-understand, likely overengineered
> > > conditional.
> > 
> > I could only reproduce the issue with display disabled. And the CI
> > machine that hits it has no monitor connected.
> > 
> > Shuicheng
> > 
> > > 
> > > Thoughts?
> > > 
> > > Matt
> > > 
> > > > > > +	if (needs_rpm && !xe_pm_runtime_get_if_active(xe))
> > > > > > {
> > > > > > +		/*
> > > > > > +		 * Resuming is not allowed from all
> > > > > > reclaim
> > > > > > contexts, so leave
> > > > > > +		 * this bo alone and ask for the device to
> > > > > > be woken
> > > > > > up outside
> > > > > > +		 * of reclaim.
> > > > > > xe_shrinker_runtime_pm_get() is gated
> > > > > > on the
> > > > > > +		 * needs of its own backup pass rather
> > > > > > than on ours,
> > > > > > and on
> > > > > > +		 * DGFX it returns before queueing
> > > > > > anything, so ask
> > > > > > here.
> > > > > > +		 */
> > > > > > +		xe_shrinker_queue_pm(xe->mem.shrinker);
> > > > > > +		goto out_unref;
> > > > > > +	}
> > > > > > +
> > > > > >  	if (xe_tt->purgeable) {
> > > > > >  		if (bo->resource->mem_type !=
> > > > > > XE_PL_SYSTEM)
> > > > > >  			lret = xe_bo_move_notify(xe_bo,
> > > > > > ctx); @@ -1369,28
> > > > > +1417,24 @@ long
> > > > > > xe_bo_shrink(struct ttm_operation_ctx *ctx, struct
> > > > > > ttm_buffer_object *bo,
> > > > > >  		if (lret > 0 &&
> > > > > > xe_bo_madv_is_dontneed(xe_bo))
> > > > > >  			xe_bo_set_purgeable_state(xe_bo,
> > > > > > 
> > > > > > XE_MADV_PURGEABLE_PURGED);
> > > > > > -		goto out_unref;
> > > > > > +		goto out_put_rpm;
> > > > > >  	}
> > > > > > 
> > > > > > -	/* System CCS needs gpu copy when moving PL_TT ->
> > > > > > PL_SYSTEM
> > > > > > */
> > > > > > -	needs_rpm = (!IS_DGFX(xe) && bo->resource-
> > > > > > >mem_type !=
> > > > > > XE_PL_SYSTEM &&
> > > > > > -		     xe_bo_needs_ccs_pages(xe_bo));
> > > > > > -	if (needs_rpm && !xe_pm_runtime_get_if_active(xe))
> > > > > > -		goto out_unref;
> > > > > > -
> > > > > >  	*scanned += tt->num_pages;
> > > > > >  	lret = ttm_bo_shrink(ctx, bo, (struct
> > > > > > ttm_bo_shrink_flags)
> > > > > >  			     {.purge = false,
> > > > > >  			      .writeback =
> > > > > > flags.writeback,
> > > > > >  			      .allow_move = true});
> > > > > > -	if (needs_rpm)
> > > > > > -		xe_pm_runtime_put(xe);
> > > > > > 
> > > > > >  	if (lret > 0) {
> > > > > >  		xe_ttm_tt_account_subtract(xe, tt);
> > > > > >  		update_global_total_pages(bo->bdev, -
> > > > > > (long)tt-
> > > > > > > num_pages);
> > > > > >  	}
> > > > > > 
> > > > > > +out_put_rpm:
> > > > > > +	if (needs_rpm)
> > > > > > +		xe_pm_runtime_put(xe);
> > > > > > +
> > > > > >  out_unref:
> > > > > >  	xe_bo_put(xe_bo);
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/xe/xe_shrinker.c
> > > > > > b/drivers/gpu/drm/xe/xe_shrinker.c
> > > > > > index 83374cd57660..fb6ec77972a5 100644
> > > > > > --- a/drivers/gpu/drm/xe/xe_shrinker.c
> > > > > > +++ b/drivers/gpu/drm/xe/xe_shrinker.c
> > > > > > @@ -54,6 +54,22 @@ xe_shrinker_mod_pages(struct xe_shrinker
> > > > > > *shrinker, long shrinkable, long purgea
> > > > > >  	write_unlock(&shrinker->lock);
> > > > > >  }
> > > > > > 
> > > > > > +/**
> > > > > > + * xe_shrinker_queue_pm() - Ask for the device to be woken
> > > > > > up for
> > > > > > shrinking
> > > > > > + * @shrinker: Pointer to the struct xe_shrinker.
> > > > > > + *
> > > > > > + * Queue a worker that takes and drops a runtime PM
> > > > > > reference.
> > > > > > Shrinking can
> > > > > > + * be called from reclaim context, where resuming the
> > > > > > device is
> > > > > > + not
> > > > > > always
> > > > > > + * allowed, so a caller that needs the device resumed but
> > > > > > could
> > > > > > + not
> > > > > > acquire a
> > > > > > + * reference uses this to have it woken up outside of
> > > > > > reclaim.
> > > > > > + The
> > > > > > current
> > > > > > + * scan makes no progress on the affected buffer objects,
> > > > > > but a
> > > > > > subsequent one
> > > > > > + * can.
> > > > > > + */
> > > > > > +void xe_shrinker_queue_pm(struct xe_shrinker *shrinker) {
> > > > > > +	queue_work(shrinker->xe->unordered_wq, &shrinker-
> > > > > > > pm_worker);
> > > > > > +}
> > > > > > +
> > > > > >  static s64 __xe_shrinker_walk(struct xe_device *xe,
> > > > > >  			      struct ttm_operation_ctx
> > > > > > *ctx,
> > > > > >  			      const struct
> > > > > > xe_bo_shrink_flags flags, @@ -185,7
> > > > > +201,7 @@
> > > > > > static bool xe_shrinker_runtime_pm_get(struct xe_shrinker
> > > > > > *shrinker, bool force,
> > > > > >  			xe_pm_runtime_get(xe);
> > > > > >  			return true;
> > > > > >  		}
> > > > > > -		queue_work(xe->unordered_wq, &shrinker-
> > > > > > >pm_worker);
> > > > > > +		xe_shrinker_queue_pm(shrinker);
> > > > > >  		return false;
> > > > > >  	}
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/xe/xe_shrinker.h
> > > > > > b/drivers/gpu/drm/xe/xe_shrinker.h
> > > > > > index 5132ae5192e1..86d2a322cadd 100644
> > > > > > --- a/drivers/gpu/drm/xe/xe_shrinker.h
> > > > > > +++ b/drivers/gpu/drm/xe/xe_shrinker.h
> > > > > > @@ -11,6 +11,8 @@ struct xe_device;
> > > > > > 
> > > > > >  void xe_shrinker_mod_pages(struct xe_shrinker *shrinker,
> > > > > > long
> > > > > > shrinkable, long purgeable);
> > > > > > 
> > > > > > +void xe_shrinker_queue_pm(struct xe_shrinker *shrinker);
> > > > > > +
> > > > > >  int xe_shrinker_create(struct xe_device *xe);
> > > > > > 
> > > > > >  #endif