Hi Daniel,
thanks for jumping in here.
And yes, you are absolutely right we need to get this fixed and not yell
at each other that we have a different understanding of things.
Your proposal sounds sane to me, but I wouldn't call it slots. Rather
something like "use cases" since we can have multiple fences for each
category I think.
And I see at four here:
1. Internal kernel memory management. Everybody needs to wait for this,
it's equal to bo->moving.
2. Writers for implicit sync, implicit sync readers should wait for them.
3. Readers for implicit sync, implicit sync writers should wait for them.
4. Things like TLB flushes and page table updates, no implicit sync but
memory management must take them into account before moving/freeing
backing store.
Happy weekend and hopefully not so much heat guys.
Cheers,
Christian.
Am 18.06.21 um 20:20 schrieb Daniel Stone:
Sorry for the mobile reply, but V4L2 is absolutely not write-only; there has never been an intersection of V4L2 supporting dmabuf and not supporting reads.
I see your point about the heritage of dma_resv but it’s a red herring. It doesn’t matter who’s right, or who was first, or where the code was extracted from.
It’s well defined that amdgpu defines resv to be one thing, that every other non-TTM user defines it to be something very different, and that the other TTM users define it to be something in the middle.
We’ll never get to anything workable if we keep arguing who’s right. Everyone is wrong, because dma_resv doesn’t globally mean anything.
It seems clear that there are three classes of synchronisation barrier (not using the ‘f’ word here), in descending exclusion order:
- memory management barriers (amdgpu exclusive fence / ttm_bo->moving)
- implicit synchronisation write barriers (everyone else’s exclusive fences, amdgpu’s shared fences)
- implicit synchronisation read barriers (everyone else’s shared fences, also amdgpu’s shared fences sometimes)
I don’t see a world in which these three uses can be reduced to two slots. What also isn’t clear to me though, is how the memory-management barriers can exclude all other access in the original proposal with purely userspace CS. Retaining the three separate modes also seems like a hard requirement to not completely break userspace, but then I don’t see how three separate slots would work if they need to be temporally ordered. amdgpu fixed this by redefining the meaning of the two slots, others fixed this by not doing one of the three modes.
So how do we square the circle without encoding a DAG into the kernel? Do the two slots need to become a single list which is ordered by time + ‘weight’ and flattened whenever modified? Something else?
Have a great weekend.
-d
On 18 Jun 2021, at 5:43 pm, Christian König <christian.koenig@xxxxxxx> wrote:
Am 18.06.21 um 17:17 schrieb Daniel Vetter:
[SNIP]
Ignoring _all_ fences is officially ok for pinned dma-buf. This is
what v4l does. Aside from it's definitely not just i915 that does this
even on the drm side, we have a few more drivers nowadays.
No it seriously isn't. If drivers are doing this they are more than broken.
See the comment in dma-resv.h
* Based on bo.c which bears the following copyright notice,
* but is dual licensed:
....
The handling in ttm_bo.c is and always was that the exclusive fence is used for buffer moves.
As I said multiple times now the *MAIN* purpose of the dma_resv object is memory management and *NOT* synchronization.
Those restrictions come from the original design of TTM where the dma_resv object originated from.
The resulting consequences are that:
a) If you access the buffer without waiting for the exclusive fence you run into a potential information leak.
We kind of let that slip for V4L since they only access the buffers for writes, so you can't do any harm there.
b) If you overwrite the exclusive fence with a new one without waiting for the old one to signal you open up the possibility for userspace to access freed up memory.
This is a complete show stopper since it means that taking over the system is just a typing exercise.
What you have done by allowing this in is ripping open a major security hole for any DMA-buf import in i915 from all TTM based driver.
This needs to be fixed ASAP, either by waiting in i915 and all other drivers doing this for the exclusive fence while importing a DMA-buf or by marking i915 and all other drivers as broken.
Sorry, but if you allowed that in you seriously have no idea what you are talking about here and where all of this originated from.
Regards,
Christian.
