Hi Matt,
On Tue, Jul 16, 2024 at 03:38:55PM +0200, Nirmoy Das wrote:GT can suspend while TLB invalidation is happening in the background. This would cause a TLB timeout when that happens. Keep the device awake when using fence which doesn't wait for the TLB invalidation to finish. Cc: Matthew Brost <matthew.brost@intel.com> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>+ Rodrigo our local PM expert.--- Adding strace here for more information: xe_pm-18095 [001] ..... 3493.481048: xe_vma_unbind: dev=0000:00:02.0, vma=ffff8881c3062b00, asid=0x0000f, start=0x0000001a0000, end=0x0000001a1fff, userptr=0x000000000000, xe_pm-18095 [001] ..... 3493.481063: xe_vm_cpu_bind: dev=0000:00:02.0, vm=ffff88812a00d000, asid=0x0000f xe_pm-18095 [001] ..... 3493.481093: xe_gt_tlb_invalidation_fence_create: dev=0000:00:02.0, fence=ffff88811bf3d000, seqno=0 xe_pm-18095 [001] ..... 3493.481095: xe_gt_tlb_invalidation_fence_work_func: dev=0000:00:02.0, fence=ffff88811bf3d000, seqno=0 xe_pm-18095 [001] ..... 3493.481097: xe_gt_tlb_TL_fence_send: dev=0000:00:02.0, fence=ffff88811bf3d000, seqno=93 xe_pm-18095 [001] d..1. 3493.481097: xe_guc_ctb_h2g: H2G CTB: dev=0000:00:02.0, gt0: action=0x7000, len=8, tail=44, head=36 kworker/1:2-17900 [001] ..... 3493.481302: xe_exec_queue_stop: dev=0000:00:02.0, 3:0x2, gt=0, width=1, guc_id=0, guc_state=0x0, flags=0x13 kworker/1:2-17900 [001] ..... 3493.481303: xe_exec_queue_stop: dev=0000:00:02.0, 3:0x1, gt=0, width=1, guc_id=1, guc_state=0x0, flags=0x4 kworker/1:2-17900 [001] ..... 3493.481305: xe_exec_queue_stop: dev=0000:00:02.0, 0:0x1, gt=0, width=1, guc_id=2, guc_state=0x0, flags=0x0 xe_pm-18095 [001] ..... 3493.756294: xe_guc_ctb_h2g: H2G CTB: dev=0000:00:02.0, gt0: action=0x3003, len=5, tail=5, head=0 xe_pm-18095 [001] d..1. 3493.756470: xe_guc_ctb_h2g: H2G CTB: dev=0000:00:02.0, gt0: action=0x3003, len=5, tail=10, head=5 kworker/u32:1-17912 [006] d..1. 3493.756535: xe_guc_ctb_g2h: G2H CTB: dev=0000:00:02.0, gt0: action=0x0, len=2, tail=2, head=2 xe_pm-18095 [001] ..... 3493.756557: xe_guc_ctb_h2g: H2G CTB: dev=0000:00:02.0, gt0: action=0x3003, len=5, tail=15, head=10 xe_pm-18095 [001] ..... 3493.756559: xe_guc_ctb_h2g: H2G CTB: dev=0000:00:02.0, gt0: action=0x3004, len=3, tail=18, head=10 kworker/1:2-17900 [001] d..1. 3497.951783: xe_gt_tlb_invalidation_fence_timeout: dev=0000:00:02.0, fence=ffff88811bf3d000, seqno=93How do you know from this the device is suspending? I can't tell that is happening. I do think this raises a good point that suspend / resume should be added to ftrace as that is useful information.
xe_exec_queue_stop() was coming from xe runtime suspend code. I am pretty sure about it but I could double check it.
drivers/gpu/drm/xe/xe_vm.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index b6932cc98ff9..241b7ea00d5f 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -2700,6 +2700,7 @@ static int vm_bind_ioctl_ops_execute(struct xe_vm *vm, struct dma_fence *fence; int err; + xe_pm_runtime_get(vm->xe);While I agree the device shouldn't enter suspend while TLB invalidations are inflight I don't think this patch will help with this. This code path is called in various places in where we should have PM ref (VM bind IOCTL, exec IOCTL for rebind, or preempt rebind worker). If we don't have PM ref when this function is called, that is a bug that needs to be fixed at the outer most layers. Beyond that, GT TLB invalidations are async and pipelined (e.g. they can be sent after this function returns and completion can returns sometime later). With this, I believe correct place to fix this is either in the CT layer or perhaps hook into GT TLB invalidation fence (Arming of fence takes a ref, signaling of fence drops a ref).
I was planning to send something more simple:
send_tlb_invalidation() --> xe_pm_runtime_get(xe);
xe_gt_tlb_fence_timeout() --> xe_pm_runtime_put(xe);
__invalidation_fence_signal() --> xe_pm_runtime_put(xe);
But that seemed too low layer for power mgmt calls. But if TLB
inval is pipelined then I agree we have to stick to a
lower layer to fix this but probably not down to CT layer.
If we choose the latter option I think following series will help as we will use GT TLB invalidation fences everywhere for waits [1]/
Regards,
Nirmoy
Rodrigo - I know we had talked about something like above but it doesn't appear this has gotten implemented. WIP or did this get lost in the PM work? Matt [1] https://patchwork.freedesktop.org/series/135809/lockdep_assert_held_write(&vm->lock); drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT | @@ -2721,6 +2722,7 @@ static int vm_bind_ioctl_ops_execute(struct xe_vm *vm, unlock: drm_exec_fini(&exec); + xe_pm_runtime_put(vm->xe); return err; } -- 2.42.0