Re: [PATCH] drm/xe/vm: Keep the device awake for TLB inval

[Nirmoy Das <nirmoy.das@linux.intel.com>]

On 7/16/2024 6:32 PM, Matthew Brost wrote:
> On Tue, Jul 16, 2024 at 06:25:01PM +0200, Nirmoy Das wrote:
>>     Hi Matt,
>>
> Outlook reply? Prefer a Linux email client for the list for proper threading.


Using Thunderbird on Windows which works most of time but I think 
sometimes it adds HTML when

pasting text.


>
>>     On 7/16/2024 5:45 PM, Matthew Brost wrote:
>>
>> 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 [1]<matthew.brost@intel.com>
>> Signed-off-by: Nirmoy Das [2]<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=fff
>> f8881c3062b00, asid=0x0000f, start=0x0000001a0000, end=0x0000001a1fff, userptr=0
>> x000000000000,
>> xe_pm-18095   [001] .....  3493.481063: xe_vm_cpu_bind: dev=0000:00:02.0, vm=fff
>> f88812a00d000, 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=000
>> 0: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_timeo
>> ut: dev=0000:00:02.0, fence=ffff88811bf3d000, seqno=93
>>
>>
>> How 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.
>>
> That would be a good idea. Any chance you want to try adding some useful
> ftrace points to get full visibility to suspend / resume flows?


Yes, I can look into that.


>   
>>
>>   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);
> The problem with this fences are currently used everywhere in the
> current code base so we'd have an imbalance. This changes that [2] but
> even with that __invalidation_fence_signal wouldn't be used in same
> places. Thus building it directly into the fence would make sense to me.
> Their are concerns about fences signaling in IRQ contexts though. I've
> pinged Rodrigo about this off the list, let's see what he thinks.


Ok, Waiting for Rodrigo's input.


Nirmoy

>
> Matt
>
> [2] https://patchwork.freedesktop.org/patch/602562/?series=135809&rev=2
>
>>
>>     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] [3]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
>>
>> References
>>
>>     1. mailto:matthew.brost@intel.com
>>     2. mailto:nirmoy.das@intel.com
>>     3. https://patchwork.freedesktop.org/series/135809/