Hi Andrey,
Am 07.04.21 um 21:44 schrieb Andrey Grodzovsky:
On 2021-04-07 6:28 a.m., Christian König wrote:
Hi Andrey,
Am 06.04.21 um 23:22 schrieb Andrey Grodzovsky:
Hey Christian, Denis, see bellow -
On 2021-04-06 6:34 a.m., Christian König wrote:
Hi Andrey,
well good question. My job is to watch over the implementation and design and while I always help I can adjust anybodies schedule.
Is the patch to print a warning when the hardware is accessed without holding the locks merged yet? If not then that would probably be a good starting point.
It's merged into amd-staging-drm-next and since I work on drm-misc-next I will cherry-pick it into there.
Ok good to know, I haven't tracked that one further.
Then we would need to unify this with the SRCU to make sure that we have both the reset lock as well as block the hotplug code from reusing the MMIO space.
In my understanding there is a significant difference between handling of GPU reset and unplug - while GPU reset use case requires any HW accessing code to block and wait for the reset to finish and then proceed, hot-unplug
is permanent and hence no need to wait and proceed but rather abort at once.
Yes, absolutely correct.
This why I think that in any place we already check for device reset we should also add a check for hot-unplug but the handling would be different
in that for hot-unplug we would abort instead of keep waiting.
Yes, that's the rough picture in my head as well.
Essentially Daniels patch of having an amdgpu_device_hwaccess_begin()/_end() was the right approach. You just can't do it in the top level IOCTL handler, but rather need it somewhere between front end and backend.
Can you point me to what patch was it ? Can't find.
What I mean was the approach in patch #3 in this series where he replaced the down_read/up_read with amdgpu_read_lock()/amdgpu_read_unlock().
I would just not call it amdgpu_read_lock()/amdgpu_read_unlock(), but something more descriptive.
Regards,
Christian.
Similar to handling device reset for unplug we obviously also need to stop and block any MMIO accesses once device is unplugged and, as Daniel Vetter mentioned - we have to do it before finishing pci_remove (early device fini)
and not later (when last device reference is dropped from user space) in order to prevent reuse of MMIO space we still access by other hot plugging devices. As in device reset case we need to cancel all delay works, stop drm schedule, complete all unfinished fences(both HW and scheduler fences). While you stated strong objection to force signalling scheduler fences from GPU reset, quote:
"you can't signal the dma_fence waiting. Waiting for a dma_fence also means you wait for the GPU reset to finish. When we would signal the dma_fence during the GPU reset then we would run into memory corruption because the hardware jobs running after the GPU reset would access memory which is already freed."
To my understating this is a key difference with hot-unplug, the device is gone, all those concerns are irrelevant and hence we can actually force signal scheduler fences (setting and error to them before) to force completion of any
waiting clients such as possibly IOCTLs or async page flips e.t.c.
Yes, absolutely correct. That's what I also mentioned to Daniel. When we are able to nuke the device and any memory access it might do we can also signal the fences.
Beyond blocking all delayed works and scheduler threads we also need to guarantee no IOCTL can access MMIO post device unplug OR in flight IOCTLs are done before we finish pci_remove (amdgpu_pci_remove for us).
For this I suggest we do something like what we worked on with Takashi Iwai the ALSA maintainer recently when he helped implementing PCI BARs move support for snd_hda_intel. Take a look at
https://cgit.freedesktop.org/~agrodzov/linux/commit/?h=yadro/pcie_hotplug/movable_bars_v9.1&id=cbaa324799718e2b828a8c7b5b001dd896748497 and
https://cgit.freedesktop.org/~agrodzov/linux/commit/?h=yadro/pcie_hotplug/movable_bars_v9.1&id=e36365d9ab5bbc30bdc221ab4b3437de34492440
We also had same issue there, how to prevent MMIO accesses while the BARs are migrating. What was done there is a refcount was added to count all IOCTLs in flight, for any in flight IOCTL the BAR migration handler would
block for the refcount to drop to 0 before it would proceed, for any later IOCTL it stops and wait if device is in migration state. We even don't need the wait part, nothing to wait for, we just return with -ENODEV for this case.
This is essentially what the DRM SRCU is doing as well.
For the hotplug case we could do this in the toplevel since we can signal the fence and don't need to block memory management.
To make SRCU 'wait for' all IOCTLs in flight we would need to wrap every IOCTL ( practically - just drm_ioctl function) with drm_dev_enter/drm_dev_exit - can we do it ?
But I'm not sure, maybe we should handle it the same way as reset or maybe we should have it at the top level.
If by top level you mean checking for device unplugged and bailing out at the entry to IOCTL or right at start of any work_item/timer function we have then seems to me it's better and more clear. Once we flushed all of them in flight there is no reason for them to execute any more when device is unplugged.