Re: [RFC 08/15] drm/xe: Implement xe_access_counter_queue_work

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

On Wed, Apr 01, 2026 at 02:10:35PM -0700, Matthew Brost wrote:
> On Wed, Mar 18, 2026 at 01:14:49PM +0530, Himal Prasad Ghimiray wrote:
> > Implement worker function to dequeue and process access counter
> > notifications and migrate bo based vma to vram and rebind it.
> > 
> > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> 
> Not a complete review - that will take a bit more time but one quick
> comment below.
> 

Sorry more thoughts in a stream.

> > ---
> >  drivers/gpu/drm/xe/xe_access_counter.c | 139 ++++++++++++++++++++++++-
> >  1 file changed, 138 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_access_counter.c b/drivers/gpu/drm/xe/xe_access_counter.c
> > index a2ce9dc45d05..9eb9917d8da7 100644
> > --- a/drivers/gpu/drm/xe/xe_access_counter.c
> > +++ b/drivers/gpu/drm/xe/xe_access_counter.c
> > @@ -11,7 +11,10 @@
> >  #include "xe_access_counter.h"
> >  #include "xe_access_counter_types.h"
> >  #include "xe_device.h"
> > +#include "xe_gt_printk.h"
> > +#include "xe_hw_engine.h"
> >  #include "xe_usm_queue.h"
> > +#include "xe_vm.h"
> >  
> >  /**
> >   * DOC: Xe access counters
> > @@ -33,9 +36,143 @@ static int xe_access_counter_entry_size(void)
> >  	return roundup_pow_of_two(sizeof(struct xe_access_counter));
> >  }
> >  
> > +static int xe_access_counter_sub_granularity_in_byte(int val)
> > +{
> > +	return xe_access_counter_granularity_in_byte(val) / 32;
> > +}
> > +
> > +static struct xe_vma *xe_access_counter_get_vma(struct xe_vm *vm,
> > +						struct xe_access_counter *ac)
> > +{
> > +	u64 page_va;
> > +
> > +	if (ac->consumer.granularity != XE_ACCESS_COUNTER_GRANULARITY_128K) {
> > +		page_va = ac->consumer.va_range_base;
> > +	} else {
> > +		page_va = ac->consumer.va_range_base +
> > +			  (ffs(ac->consumer.sub_granularity) - 1) *
> > +			  xe_access_counter_sub_granularity_in_byte(ac->consumer.granularity);
> > +	}
> > +
> > +	return xe_vm_find_overlapping_vma(vm, page_va, SZ_4K);
> > +}
> > +
> > +static void xe_access_counter_print(struct xe_access_counter *ac)
> > +{
> > +	xe_gt_dbg(ac->gt, "\n\tASID: %d\n"
> > +		  "\tVA Range Base: 0x%08x%08x\n"
> > +		  "\tCounter Type: %d\n"
> > +		  "\tGranularity: %d\n"
> > +		  "\tSub-Granularity: 0x%08x\n"
> > +		  "\tEngineClass: %d %s\n"
> > +		  "\tEngineInstance: %d\n",
> > +		  ac->consumer.xe3.asid,
> > +		  upper_32_bits(ac->consumer.va_range_base),
> > +		  lower_32_bits(ac->consumer.va_range_base),
> > +		  ac->consumer.counter_type,
> > +		  ac->consumer.granularity,
> > +		  ac->consumer.sub_granularity,
> > +		  ac->consumer.xe3.engine_class,
> > +		  xe_hw_engine_class_to_str(ac->consumer.xe3.engine_class),
> > +		  ac->consumer.xe3.engine_instance);
> > +}
> > +
> > +static int xe_access_counter_service(struct xe_access_counter *ac)
> > +{
> > +	struct xe_gt *gt = ac->gt;
> > +	struct xe_device *xe = gt_to_xe(gt);
> > +	struct xe_tile *tile = gt_to_tile(gt);
> > +	struct xe_validation_ctx ctx;
> > +	struct drm_exec exec;
> > +	struct dma_fence *fence;
> > +	struct xe_vm *vm;
> > +	struct xe_vma *vma;
> > +	int err = 0;
> > +
> > +	if (ac->consumer.counter_type > XE_ACCESS_COUNTER_TYPE_NOTIFY)
> > +		return -EINVAL;
> > +
> > +	vm = xe_device_asid_to_fault_vm(xe, ac->consumer.xe3.asid);
> > +	if (IS_ERR(vm))
> > +		return PTR_ERR(vm);
> > +
> > +	down_write(&vm->lock);
> > +
> > +	if (xe_vm_is_closed(vm)) {
> > +		err = -ENOENT;
> > +		goto unlock_vm;
> > +	}
> > +	/* Lookup VMA */
> > +	vma = xe_access_counter_get_vma(vm, ac);
> > +	if (!vma) {
> > +		err = -EINVAL;
> > +		goto unlock_vm;
> > +	}
> > +
> > +	/* TODO: Handle svm vma's  */
> > +	if (xe_vma_has_no_bo(vma))
> > +		goto unlock_vm;
> > +
> > +	/* Lock VM and BOs dma-resv */
> > +	xe_validation_ctx_init(&ctx, &vm->xe->val, &exec, (struct xe_val_flags) {});
> > +	drm_exec_until_all_locked(&exec) {
> > +		err = xe_vma_lock_and_validate(&exec, vma, tile->mem.vram, true);
> > +		drm_exec_retry_on_contention(&exec);
> > +		xe_validation_retry_on_oom(&ctx, &err);
> > +		if (err)
> > +			break;
> > +
> > +		xe_vm_set_validation_exec(vm, &exec);
> > +		fence = xe_vma_rebind(vm, vma, BIT(tile->id));
> > +		xe_vm_set_validation_exec(vm, NULL);
> > +		if (IS_ERR(fence))
> > +			err = PTR_ERR(fence);
> > +	}
> > +
> > +	if (!err && !IS_ERR(fence)) {
> > +		dma_fence_wait(fence, false);
> > +		dma_fence_put(fence);
> > +	}
> 
> You can move the dma_fence_wait()/put outside all of the locks taken here.
> xe_pagefault_handle_vma() also unnecessarily waits under locks, but that
> will be refactored a bit in [1]. Even there, I should probably move the
> dma_fence_wait() completely outside vm->lock, which is held in read mode
> after [1].
> 
> Best to get the semantics of this new layer right upfront.
> 

Actually, now that I think about it, you never need to wait here, given
that migrations and binds are fully pipelined operations and we don’t
ack an access-counter event with an H2G.

What will race is a possible page fault while we’re moving the VMA’s
memory. So I think we should just attach a fence to the VMA (under
vm->lock in write mode for now), which the page-fault handler can wait
on before the xe_vm_has_valid_gpu_mapping check in
xe_pagefault_handle_vma(). You can drop the fence there, or if a VMA
still has a fence when it is destroyed, drop the reference at that
point. This should be a relatively small amount of memory (for the
fence), so if we don’t free it for a while, I don’t think it’s a big
deal..

Another hazard is when multiple access-counter events map to the same
VMA, so if an unsignaled ‘access-counter move’ fence is found, we should
simply drop the access-counter event.

> Matt
> 
> [1] https://patchwork.freedesktop.org/patch/707294/?series=162167&rev=4
> 
> > +
> > +	xe_validation_ctx_fini(&ctx);
> > +
> > +unlock_vm:
> > +	up_write(&vm->lock);
> > +	xe_vm_put(vm);
> > +
> > +	return err;
> > +}
> > +
> >  static void xe_access_counter_queue_work_func(struct work_struct *w)
> >  {
> > -	/* TODO: Implement */
> > +	struct xe_usm_queue *ac_queue =
> > +		container_of(w, typeof(*ac_queue), worker);
> > +	struct xe_access_counter ac = {};
> > +	unsigned long threshold;
> > +
> > +#define USM_QUEUE_MAX_RUNTIME_MS	20
> > +	threshold = jiffies + msecs_to_jiffies(USM_QUEUE_MAX_RUNTIME_MS);
> > +
> > +	while (xe_usm_queue_pop(ac_queue, &ac, xe_access_counter_entry_size())) {
> > +		int err;
> > +

So here - as discussed [1], let's have a single queue /w multiple workers
pulling access counters events.

[1] https://patchwork.freedesktop.org/patch/712591/?series=163429&rev=1

> > +		if (!ac.gt)	/* Access counter squashed during reset */
> > +			continue;
> > +
> > +		err = xe_access_counter_service(&ac);
> > +		if (err) {
> > +			xe_access_counter_print(&ac);
> > +			xe_gt_dbg(ac.gt, "Access counter handling: Unsuccessful %pe\n",
> > +				  ERR_PTR(err));
> > +		}
> > +
> > +		if (time_after(jiffies, threshold) &&
> > +		    ac_queue->tail != ac_queue->head) {
> > +			queue_work(gt_to_xe(ac.gt)->usm.pf_wq, w);

I think scheduling access-counter events on gt_to_xe(ac.gt)->usm.pf_wq
is correct, but since this is shared with page faults—and those are
always higher priority—we should service at most one access-counter
event here to give page-fault work items a chance to jump the line.

So maybe something like this instead of a while loop:

if (xe_usm_queue_peak())
	queue_work();

Matt

> > +			break;
> > +		}
> > +	}
> > +#undef USM_QUEUE_MAX_RUNTIME_MS
> >  }
> >  
> >  static int xe_access_counter_queue_init(struct xe_device *xe,
> > -- 
> > 2.34.1
> >