On Thu, Aug 15, 2019 at 03:01:59PM -0300, Jason Gunthorpe wrote: > On Thu, Aug 15, 2019 at 01:39:22PM -0400, Jerome Glisse wrote: > > On Thu, Aug 15, 2019 at 02:35:57PM -0300, Jason Gunthorpe wrote: > > > On Thu, Aug 15, 2019 at 06:25:16PM +0200, Daniel Vetter wrote: > > > > > > > I'm not really well versed in the details of our userptr, but both > > > > amdgpu and i915 wait for the gpu to complete from > > > > invalidate_range_start. Jerome has at least looked a lot at the amdgpu > > > > one, so maybe he can explain what exactly it is we're doing ... > > > > > > amdgpu is (wrongly) using hmm for something, I can't really tell what > > > it is trying to do. The calls to dma_fence under the > > > invalidate_range_start do not give me a good feeling. > > > > > > However, i915 shows all the signs of trying to follow the registration > > > cache model, it even has a nice comment in > > > i915_gem_userptr_get_pages() explaining that the races it has don't > > > matter because it is a user space bug to change the VA mapping in the > > > first place. That just screams registration cache to me. > > > > > > So it is fine to run HW that way, but if you do, there is no reason to > > > fence inside the invalidate_range end. Just orphan the DMA buffer and > > > clean it up & release the page pins when all DMA buffer refs go to > > > zero. The next access to that VA should get a new DMA buffer with the > > > right mapping. > > > > > > In other words the invalidation should be very simple without > > > complicated locking, or wait_event's. Look at hfi1 for example. > > > > This would break the today usage model of uptr and it will > > break userspace expectation ie if GPU is writting to that > > memory and that memory then the userspace want to make sure > > that it will see what the GPU write. > > How exactly? This is holding the page pin, so the only way the VA > mapping can be changed is via explicit user action. > > ie: > > gpu_write_something(va, size) > mmap(.., va, size, MMAP_FIXED); > gpu_wait_done() > > This is racy and indeterminate with both models. > > Based on the comment in i915 it appears to be going on the model that > changes to the mmap by userspace when the GPU is working on it is a > programming bug. This is reasonable, lots of systems use this kind of > consistency model. Well userspace process doing munmap(), mremap(), fork() and things like that are a bug from the i915 kernel and userspace contract point of view. But things like migration or reclaim are not cover under that contract and for those the expectation is that CPU access to the same virtual address should allow to get what was last written to it either by the GPU or the CPU. > > Since the driver seems to rely on a dma_fence to block DMA access, it > looks to me like the kernel has full visibility to the > 'gpu_write_something()' and 'gpu_wait_done()' markers. So let's only consider the case where GPU wants to write to the memory (the read only case is obviously right and not need any notifier in fact) and like above the only thing we care about is reclaim or migration (for instance because of some numa compaction) as the rest is cover by i915 userspace contract. So in the write case we do GUPfast(write=true) which will be "safe" from any concurrent CPU page table update ie if GUPfast get a reference for the page then any racing CPU page table update will not be able to migrate or reclaim the page and thus the virtual address to page association will be preserve. This is only true because of GUPfast(), now if GUPfast() fails it will fallback to the slow GUP case which make the same thing safe by taking the page table lock. Because of the reference on the page the i915 driver can forego the mmu notifier end callback. The thing here is that taking a page reference is pointless if we have better synchronization and tracking of mmu notifier. Hence converting to hmm mirror allows to avoid taking a ref on the page while still keeping the same functionality as of today. > I think trying to use hmm_range_fault on HW that can't do HW page > faulting and HW 'TLB shootdown' is a very, very bad idea. I fear that > is what amd gpu is trying to do. > > I haven't yet seen anything that looks like 'TLB shootdown' in i915?? GPU driver have complex usage pattern the tlb shootdown is implicit once the GEM object associated with the uptr is invalidated it means next time userspace submit command against that GEM object it will have to re-validate it which means re-program the GPU page table to point to the proper address (and re-call GUP). So the whole GPU page table update is all hidden behind GEM function like i915_gem_object_unbind() (or ttm invalidate for amd/radeon). Technicaly those GPU do not support page faulting but because of the way GPU works you know that you have frequent checkpoint where you can unbind things. This is what is happening here. Also not all GPU engines can handle page fault, this is true of all GPU with page fault AFAIK (AMD and NVidia so far). This is why uptr is a limited form of SVM (share virtual memory) that can be use on all GPUs engine including the dma engine. When using the full SVM power (like hmm mirror with nouveau) this is only use in GPU engine that supports page fault (but updating the page table still require locking and waiting on acknowledge). Cheers, Jérôme _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
