On Wed, Feb 26, 2020 at 4:05 AM Daniel Vetter <daniel@xxxxxxxx> wrote: > > On Wed, Feb 26, 2020 at 10:16:05AM +0100, Christian König wrote: > > Hi Jason, > > > > Am 26.02.20 um 00:58 schrieb Jason Ekstrand: > > > Explicit synchronization is the future. At least, that seems to be what > > > most userspace APIs are agreeing on at this point. However, most of our > > > Linux APIs (both userspace and kernel UAPI) are currently built around > > > implicit synchronization with dma-buf. While work is ongoing to change > > > many of the userspace APIs and protocols to an explicit synchronization > > > model, switching over piecemeal is difficult due to the number of > > > potential components involved. On the kernel side, many drivers use > > > dma-buf including GPU (3D/compute), display, v4l, and others. In > > > userspace, we have X11, several Wayland compositors, 3D drivers, compute > > > drivers (OpenCL etc.), media encode/decode, and the list goes on. > > > > > > This patch provides a path forward by allowing userspace to manually > > > manage the fences attached to a dma-buf. Alternatively, one can think > > > of this as making dma-buf's implicit synchronization simply a carrier > > > for an explicit fence. This is accomplished by adding two IOCTLs to > > > dma-buf for importing and exporting a sync file to/from the dma-buf. > > > This way a userspace component which is uses explicit synchronization, > > > such as a Vulkan driver, can manually set the write fence on a buffer > > > before handing it off to an implicitly synchronized component such as a > > > Wayland compositor or video encoder. In this way, each of the different > > > components can be upgraded to an explicit synchronization model one at a > > > time as long as the userspace pieces connecting them are aware of it and > > > import/export fences at the right times. > > > > > > There is a potential race condition with this API if userspace is not > > > careful. A typical use case for implicit synchronization is to wait for > > > the dma-buf to be ready, use it, and then signal it for some other > > > component. Because a sync_file cannot be created until it is guaranteed > > > to complete in finite time, userspace can only signal the dma-buf after > > > it has already submitted the work which uses it to the kernel and has > > > received a sync_file back. There is no way to atomically submit a > > > wait-use-signal operation. This is not, however, really a problem with > > > this API so much as it is a problem with explicit synchronization > > > itself. The way this is typically handled is to have very explicit > > > ownership transfer points in the API or protocol which ensure that only > > > one component is using it at any given time. Both X11 (via the PRESENT > > > extension) and Wayland provide such ownership transfer points via > > > explicit present and idle messages. > > > > > > The decision was intentionally made in this patch to make the import and > > > export operations IOCTLs on the dma-buf itself rather than as a DRM > > > IOCTL. This makes it the import/export operation universal across all > > > components which use dma-buf including GPU, display, v4l, and others. > > > It also means that a userspace component can do the import/export > > > without access to the DRM fd which may be tricky to get in cases where > > > the client communicates with DRM via a userspace API such as OpenGL or > > > Vulkan. At a future date we may choose to add direct import/export APIs > > > to components such as drm_syncobj to avoid allocating a file descriptor > > > and going through two ioctls. However, that seems to be something of a > > > micro-optimization as import/export operations are likely to happen at a > > > rate of a few per frame of rendered or decoded video. > > > > > > Signed-off-by: Jason Ekstrand <jason@xxxxxxxxxxxxxx> > > > --- > > > > > > This is marked as an RFC because I intend it to start a discussion about > > > how to solve a problem. The current patch compiles but that's it for now. > > > I'll be writing IGT tests and Vulkan driver patches which exercise it over > > > the next couple of days. In the mean time, feel free to tell me why you > > > think this is a great and/or terrible idea. :-) > > > > For the exporting part I think it is an absolutely great idea because it > > simplifies compatibility with explicit sync quite a bit. Unfortunately, it only helps half of explicit sync and not the half that's hard to deal with from Vulkan. :-/ > > But for the importing part it is a clear NAK at the moment. See we can't > > allow userspace to mess with DMA-buf fences in that way because it rips open > > a security hole you can push an elephant through. Oh, sure, I'm 100% sure I did that part wrong. Why else would I send the patch but to have someone who actually knows what they're doing tell me how to do it correctly? :-P > > Just imagine that you access some DMA-buf with a shader and that operation > > is presented as a fence on the DMA-bufs reservation object. And now you can > > go ahead and replace that fence and free up the memory. > > > > Tricking the Linux kernel into allocating page tables in that freed memory > > is trivial and that's basically it you can overwrite page tables with your > > shader and gain access to all of system memory :) > > > > What we could do is to always make sure that the added fences will complete > > later than the already existing ones, but that is also rather tricky to get > > right. I wouldn't do that if we don't have a rather big use case for this. Right. I thought about that but I'm still learning how dma_resv works. It'd be easy enough to make a fence array that contains both the old fence and the new fence and replace the old fence with that. What I don't know is the proper way to replace the exclusive fence safely. Some sort of atomic_cpxchg loop, perhaps? I presume there's some way of doing it properly because DRM drivers are doing it all the time. > I think the main use-case for adding a fence is adding a write fence for > vk winsys buffers, which run without any sync at all. So essentially what > we'd do is promote one of the read fences which are already attached to be > the write fence. Correct. We're effectively doing an import in ANV today but we're doing it with a dummy execbuf which claims to write the BO and has a batch that's just MI_BATCH_BUFFER_END. > But yeah making sure we don't break any of the dma_resv guarantees about > how these fences works is going to be somewhat tricky. Probably can reuse > a big chunk of the fence container work we've done for syncobj timelines, > since they have some of the same issues of having to chain fences to not > break the world. Happy to not break the world. I just don't know how yet. :-) --Jason
