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

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

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.

Matt

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