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

[Christian König]

>>>
>>>> [SNIP]
>>>>>>>
>>>>>>> Maybe just empirically - let's try it and see under different 
>>>>>>> test scenarios what actually happens  ?
>>>>>>
>>>>>> Not a good idea in general, we have that approach way to often at 
>>>>>> AMD and are then surprised that everything works in QA but fails 
>>>>>> in production.
>>>>>>
>>>>>> But Daniel already noted in his reply that waiting for a fence 
>>>>>> while holding the SRCU is expected to work.
>>>>>>
>>>>>> So let's stick with the approach of high level locking for hotplug.
>>>>>
>>>>>
>>>>> To my understanding this is true for other devises, not the one 
>>>>> being extracted, for him you still need to do all the HW fence 
>>>>> signalling dance because the HW is gone and we block any TDRs 
>>>>> (which won't help anyway).
>>>>>
>>>>> Andrey
>>>
>>>
>>> Do you agree to the above ?
>>
>> Yeah, I think that is correct.
>>
>> But on the other hand what Daniel reminded me of is that the handling 
>> needs to be consistent over different devices. And since some device 
>> already go with the approach of canceling everything we simply have 
>> to go down that route as well.
>>
>> Christian.
>
>
> What does it mean in our context ? What needs to be done which we are 
> not doing now ?

I think we are fine, we just need to continue with the approach of 
forcefully signaling all fences on hotplug.

Christian.

>
> Andrey
>
>
>>
>>>
>>> Andrey
>>>
>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>> Andrey
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Andrey
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Christian.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Andrey
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> BTW: Could it be that the device SRCU protects more 
>>>>>>>>>>>>>>>> than one device and we deadlock because of this?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I haven't actually experienced any deadlock until now 
>>>>>>>>>>>>>>> but, yes, drm_unplug_srcu is defined as static in 
>>>>>>>>>>>>>>> drm_drv.c and so in the presence of multiple devices 
>>>>>>>>>>>>>>> from same or different drivers we in fact are dependent 
>>>>>>>>>>>>>>> on all their critical sections i guess.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Shit, yeah the devil is a squirrel. So for A+I laptops we 
>>>>>>>>>>>>>> actually need to sync that up with Daniel and the rest of 
>>>>>>>>>>>>>> the i915 guys.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> IIRC we could actually have an amdgpu device in a docking 
>>>>>>>>>>>>>> station which needs hotplug and the driver might depend 
>>>>>>>>>>>>>> on waiting for the i915 driver as well.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Can't we propose a patch to make drm_unplug_srcu per 
>>>>>>>>>>>>> drm_device ? I don't see why it has to be global and not 
>>>>>>>>>>>>> per device thing.
>>>>>>>>>>>>
>>>>>>>>>>>> I'm really wondering the same thing for quite a while now.
>>>>>>>>>>>>
>>>>>>>>>>>> Adding Daniel as well, maybe he knows why the 
>>>>>>>>>>>> drm_unplug_srcu is global.
>>>>>>>>>>>>
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Christian.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Andrey
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Christian.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Andrey
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Christian.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Andrey
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Andrey
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Christian.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> /* Past this point no more fence are submitted to 
>>>>>>>>>>>>>>>>>>>>> HW ring and hence we can safely call force signal 
>>>>>>>>>>>>>>>>>>>>> on all that are currently there.
>>>>>>>>>>>>>>>>>>>>>      * Any subsequently created HW fences will be 
>>>>>>>>>>>>>>>>>>>>> returned signaled with an error code right away
>>>>>>>>>>>>>>>>>>>>>      */
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>     for_each_ring(adev)
>>>>>>>>>>>>>>>>>>>>> amdgpu_fence_process(ring)
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>     drm_dev_unplug(dev);
>>>>>>>>>>>>>>>>>>>>>     Stop schedulers
>>>>>>>>>>>>>>>>>>>>>     cancel_sync(all timers and queued works);
>>>>>>>>>>>>>>>>>>>>>     hw_fini
>>>>>>>>>>>>>>>>>>>>>     unmap_mmio
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Andrey
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> 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://nam11.safelinks.protection.outlook.com/?url=https:%2F%2Fcgit.freedesktop.org%2F~agrodzov%2Flinux%2Fcommit%2F%3Fh%3Ddrm-misc-next%26id%3Def0ea4dd29ef44d2649c5eda16c8f4869acc36b1&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Ca64b1f5e0df0403a656408d8ffdc7bdb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637540669732692484%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=pLcplnlDIESV998tLO7iydxEo5lh71BjQCbAOxKif2Q%3D&reserved=0 
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Yes, good point as well.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Christian.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Andrey
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Christian.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Andrey
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Christian.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Andrey
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Andrey
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>