Don't install exec fences into extobj BO's write slots unconditionally

@llandwerlin @kwg @thomash @bbrezillon

TL;DR: Looks like Xe got installing exec fences into extobj BO's write slots unconditionally wrong we should just be using bookkeep with user space using dma-buf api [1] to set this. Could unnecessarily slow down a compositor. Panthor looks to have gotten this wrong too by copying Xe.

Rough plan to fix this:

• Try fixing kernel to use bookkeep slots, if mesa / compositors don't break we are done.
• If mesa / compositor break, add VM flag to change behavior, fix mesa / compositors, merge.

[1] https://patchwork.freedesktop.org/series/104898/

Chat on #dri-devel

<sima> hm just noticed that xe unconditionally set the write fence for all external objs
<sima> kinda defeats all the explicit sync work for vulkan, did no one yet complain?
<sima> also a pile of other places even for gl with winsys buffers
<airlied> I've a vague memory of that topic coming up on the list
<mbrost> sima, airlied: I believe for extobjs fences are installed in dma-resv write slots to get the dma-buf api to work. e.g. what xe_dma_buf_sync.c is testing
<mbrost> since we have no idea is an exec is read or write operation or which extobjs each exec touches with our exec uAPI we unconditionally install the exec fence in all extobj write dma-resv slots 
<airlied> https://lore.kernel.org/dri-devel/ZPHUuU4L4P2A8gan@cassiopeiae/
<airlied> somewhere there maybe
<mbrost> let me know if this thinking is wrong or alternative idea works
<mbrost> airlied: let me read that
<airlied> not sure if I'm conflating discussions, my in-brain indexer is fuzzy :-)
<airlied> "> I think we're then optimizing for different scenarios. Our compute driver
<airlied> > will use mostly external objects only,
<sima> mbrost, yeah that's why we have the dma_buf fence import/export ioctl, so that userspace can set these up correctly
<sima> i915-gem somewhat got this right by at least allowing only READ fences
<sima> compute-runtime might not keep track of stuff and need this, but vulkan must do this right or it's broken (afaik)
<mbrost> we only install fences for vulkan (non-LR VMs)
<mbrost> we don't even take dma-resv lock for LR VMs on exec
<sima> hm right the preempt fence gets reinstalled as USAGE_BOOKKEEP for everything
<sima> looks even more funny
<sima> airlied, ^^ so it's not compute
<sima> or at least not long-running compute ctx
<sima> anyway it's not disaster, just kinda funny that xe is worse than i915-gem here when we now have all the infra to make this perfect for vulkan umd
<mbrost> i915-gem has a BO list though with R/W permissions, Xe doesn't
<mbrost> internally difference between bookkeep and write on an exec doesn't really matter, both cases block the move of the BO, afiak read / write only show up when exporting via dma-buf and another process extracts sync to wait on
<sima> vulkan submits everything in every ioctl
<sima> for i915-gem
<sima> and yeah it's exactly for shared bo where unconditionally setting write on every job is wrong
<sima> setting read on everything is also not great, but much better
<sima> and yeah from the kernel pov bookkeep/read/write are all the same really
<sima> example: you have 2 buffers shared with the compositor
<sima> one you render into, the other the compositor reads
<sima> if you always set a write fence, then every time the compositor reads it will block on whatever rendering you happen to have currently queued
<sima> that's ... broken
<mbrost> hmm... maybe I'm not understanding this. Render is writing the BO, doesn't the compositor need to wait for that...
<sima> you have 2 buffers
<sima> you should only set the write fence for the buffer you're actually rendering into, not both of them all the time
<sima> like this is so busted xe and panthor (which copied xe) are the only two drivers that get this this wrong
<mbrost> how would Xe know which BO it is writing too?
<sima> it doesn't
<sima> it's userspace job, using the dma_buf import/export ioctl that gfxstrand added
<sima> so for vulkan it's roughly
<sima> 1. you submit lots and lots of rendering, this only ever sets bookkeeping fences
<sima> 2. when the app is ready to submit a new frame to the compositor it tells the vulkan driver a) the buffer b) the fence that signals when all writing has finished
<mbrost> oh, you mean user space should install the write fences not Xe
<sima> vulkan driver calls DMA_BUF_IOCTL_IMPORT_SYNC_FILE on that dma-buf shared with the compositor, and then does the winsys protocal call to inform the compositor
<sima> yup
<sima> similar on the read side
<sima> gl is a bit dumber, because the rule is that unless you've opted into vk-style explicit fence you need to install fences around every exec ioctl call
<mbrost> render gets a fence from an exec and sets the write fences in the BO
<sima> yup
<mbrost> ah, ok
<mbrost> let me follow up on that and see if mesa is doing that
<sima> similar on the compositor side of things, assuming vk compositor
<sima> 1. compositor gets a new dma-buf frame
<sima> 2. it fishes out the right fences with DMA_BUF_IOCTL_EXPORT_SYNC_FILE
<mbrost> it might not because Xe is doing this
<sima> 3. it hands that fence to vulkan for synchronization, vulkan should _not_ wait on implicit fences on its own unless told to
<sima> mbrost, yeah good chances mesa didn't fix their code (it's for sure not tested), so likely broken
<sima> and since i915-gem sets read fences at least, there's still some amount of implicit sync happening compared to pure bookkeeping fences
<sima> so if you're lucky, you might switch to read fences without an explicit ctx creation opt-in flag
<sima> and it'll work, because that should match i915-gem
<mbrost> i'll open a gitlab issue now, yea if this is broken we should get this fixed in 6.12 or may even have to add a VM flag I suppose to change this
<sima> but really the idea of adding the dma-buf import/export ioctl was to go with bookkeeping fences only
<sima> and leave implicit sync to userspace as something it has to do explicitly
<mbrost> ok, missed that. I think I added write sematic like 2 year ago and misunderstood this
<sima> yeah it's the easy hack to get winsys sync going
<sima> also iirc for vulkan too much write sync is actually broken per the spec, some games don't like that
<sima> mbrost, also for patches pls cc bbrezillion/panthor folks, since they seem to be confused too
<sima> or on the gitlab issue