[PATCH 0/4] Refine GPU recovery sequence to enhance its stability

[Christian König]

Hi Andrey,

Am 09.04.21 um 20:18 schrieb Andrey Grodzovsky:
> [SNIP]
>>>
>>> If we use a list and a flag called 'emit_allowed' under a lock such 
>>> that in amdgpu_fence_emit we lock the list, check the flag and if 
>>> true add the new HW fence to list and proceed to HW emition as 
>>> normal, otherwise return with -ENODEV. In amdgpu_pci_remove we take 
>>> the lock, set the flag to false, and then iterate the list and force 
>>> signal it. Will this not prevent any new HW fence creation from now 
>>> on from any place trying to do so ?
>>
>> Way to much overhead. The fence processing is intentionally lock free 
>> to avoid cache line bouncing because the IRQ can move from CPU to CPU.
>>
>> We need something which at least the processing of fences in the 
>> interrupt handler doesn't affect at all.
>
>
> As far as I see in the code, amdgpu_fence_emit is only called from 
> task context. Also, we can skip this list I proposed and just use 
> amdgpu_fence_driver_force_completion for each ring to signal all 
> created HW fences.

Ah, wait a second this gave me another idea.

See amdgpu_fence_driver_force_completion():

amdgpu_fence_write(ring, ring->fence_drv.sync_seq);

If we change that to something like:

amdgpu_fence_write(ring, ring->fence_drv.sync_seq + 0x3FFFFFFF);

Not only the currently submitted, but also the next 0x3FFFFFFF fences 
will be considered signaled.

This basically solves out problem of making sure that new fences are 
also signaled without any additional overhead whatsoever.

>
>>>>
>>>> Alternatively grabbing the reset write side and stopping and then 
>>>> restarting the scheduler could work as well.
>>>>
>>>> Christian.
>>>
>>>
>>> I didn't get the above and I don't see why I need to reuse the GPU 
>>> reset rw_lock. I rely on the SRCU unplug flag for unplug. Also, not 
>>> clear to me why are we focusing on the scheduler threads, any code 
>>> patch to generate HW fences should be covered, so any code leading 
>>> to amdgpu_fence_emit needs to be taken into account such as, direct 
>>> IB submissions, VM flushes e.t.c
>>
>> You need to work together with the reset lock anyway, cause a hotplug 
>> could run at the same time as a reset.
>
>
> For going my way indeed now I see now that I have to take reset write 
> side lock during HW fences signalling in order to protect against 
> scheduler/HW fences detachment and reattachment during schedulers 
> stop/restart. But if we go with your approach  then calling 
> drm_dev_unplug and scoping amdgpu_job_timeout with drm_dev_enter/exit 
> should be enough to prevent any concurrent GPU resets during unplug. 
> In fact I already do it anyway - 
> https://cgit.freedesktop.org/~agrodzov/linux/commit/?h=drm-misc-next&id=ef0ea4dd29ef44d2649c5eda16c8f4869acc36b1

Yes, good point as well.

Christian.

>
> Andrey
>
>
>>
>>
>> Christian.
>>
>>>
>>> Andrey
>>>
>>>
>>>>
>>>>>
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Andrey
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>> Andrey
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>