On Fri, Feb 5, 2021 at 5:22 PM Andrey Grodzovsky <Andrey.Grodzovsky@xxxxxxx> wrote: > > Daniel, ping. Also, please refer to the other thread with Bjorn from pci-dev > on the same topic I added you to. Summarizing my take over there for here plus maybe some more clarification. There's two problems: - You must guarantee that after the ->remove callback of your driver is finished, there's no more mmio or any other hw access. A combination of stopping stuff and drm_dev_enter/exit can help with that. This prevents the use-after-free issue. - For the actual hotunplug time, i.e. anything that can run while your driver is used up to the point where ->remove callback has finished stopp hw access you must guarantee that code doesn't blow up when it gets bogus reads (in the form of 0xff values). drm_dev_enter/exit can't help you with that. Plus you should make sure that we're not spending forever waiting for a big pile of mmio access all to time out because you never bail out - some coarse-grained drm_dev_enter/exit might help here. Plus finally the userspace access problem: You must guarantee that after ->remove has finished that none of the uapi or cross-driver access points (driver ioctl, dma-buf, dma-fence, anything else that hangs around) can reach the data structures/memory mappings/whatever which have been released as part of your ->remove callback. drm_dev_enter/exit is again the tool of choice here. So you have to use drm_dev_enter/exit for some of the problems we face on hotunplug, but it's not the tool that can handle the actual hw hotunplug race conditions for you. Unfortunately the hw hotunplug race condition is an utter pain to test, since essentially you need to validate your driver against spurious 0xff reads at any moment. And I don't even have a clever idea to simulate this, e.g. by forcefully replacing the iobar mapping: What we'd need is a mapping that allows reads (so we can fill a page with 0xff and use that everywhere), but instead of rejecting writes, allows them, but drops them (so that the 0xff stays intact). Maybe we could simulate this with some kernel debug tricks (kinda like mmiotrace) with a read-only mapping and dropping every write every time we fault. But ugh ... Otoh validating an entire driver like amdgpu without such a trick against 0xff reads is practically impossible. So maybe you need to add this as one of the tasks here? -Daniel > > Andrey > > On 1/29/21 2:25 PM, Christian König wrote: > > Am 29.01.21 um 18:35 schrieb Andrey Grodzovsky: > >> > >> On 1/29/21 10:16 AM, Christian König wrote: > >>> Am 28.01.21 um 18:23 schrieb Andrey Grodzovsky: > >>>> > >>>> On 1/19/21 1:59 PM, Christian König wrote: > >>>>> Am 19.01.21 um 19:22 schrieb Andrey Grodzovsky: > >>>>>> > >>>>>> On 1/19/21 1:05 PM, Daniel Vetter wrote: > >>>>>>> [SNIP] > >>>>>> So say writing in a loop to some harmless scratch register for many times > >>>>>> both for plugged > >>>>>> and unplugged case and measure total time delta ? > >>>>> > >>>>> I think we should at least measure the following: > >>>>> > >>>>> 1. Writing X times to a scratch reg without your patch. > >>>>> 2. Writing X times to a scratch reg with your patch. > >>>>> 3. Writing X times to a scratch reg with the hardware physically disconnected. > >>>>> > >>>>> I suggest to repeat that once for Polaris (or older) and once for Vega or > >>>>> Navi. > >>>>> > >>>>> The SRBM on Polaris is meant to introduce some delay in each access, so it > >>>>> might react differently then the newer hardware. > >>>>> > >>>>> Christian. > >>>> > >>>> > >>>> See attached results and the testing code. Ran on Polaris (gfx8) and > >>>> Vega10(gfx9) > >>>> > >>>> In summary, over 1 million WWREG32 in loop with and without this patch you > >>>> get around 10ms of accumulated overhead ( so 0.00001 millisecond penalty for > >>>> each WWREG32) for using drm_dev_enter check when writing registers. > >>>> > >>>> P.S Bullet 3 I cannot test as I need eGPU and currently I don't have one. > >>> > >>> Well if I'm not completely mistaken that are 100ms of accumulated overhead. > >>> So around 100ns per write. And even bigger problem is that this is a ~67% > >>> increase. > >> > >> > >> My bad, and 67% from what ? How u calculate ? > > > > My bad, (308501-209689)/209689=47% increase. > > > >>> > >>> I'm not sure how many write we do during normal operation, but that sounds > >>> like a bit much. Ideas? > >> > >> Well, u suggested to move the drm_dev_enter way up but as i see it the problem > >> with this is that it increase the chance of race where the > >> device is extracted after we check for drm_dev_enter (there is also such > >> chance even when it's placed inside WWREG but it's lower). > >> Earlier I propsed that instead of doing all those guards scattered all over > >> the code simply delay release of system memory pages and unreserve of > >> MMIO ranges to until after the device itself is gone after last drm device > >> reference is dropped. But Daniel opposes delaying MMIO ranges unreserve to after > >> PCI remove code because according to him it will upset the PCI subsytem. > > > > Yeah, that's most likely true as well. > > > > Maybe Daniel has another idea when he's back from vacation. > > > > Christian. > > > >> > >> Andrey > >> > >>> > >>> Christian. > >> _______________________________________________ > >> amd-gfx mailing list > >> amd-gfx@xxxxxxxxxxxxxxxxxxxxx > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7CAndrey.Grodzovsky%40amd.com%7C7e63c7ba9ac44d80163108d8c48b9507%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637475451078731703%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=SozIYYmHpkk%2B4PRycs8T7x1DYagThy6lQoFXV5Ddamk%3D&reserved=0 > >> > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
