Hi Daniel,
Am 25.06.22 um 00:02 schrieb Daniel Vetter:
On Thu, Jun 23, 2022 at 01:32:18PM +0200, Christian König wrote:
Am 23.06.22 um 13:27 schrieb Daniel Stone:
[SNIP]
If it's really your belief that dmabuf requires universal snooping, I
recommend you send the patch to update the documentation, as well as
to remove DRIVER_PRIME from, realistically, most non-PCIE drivers.
Well, to be honest I think that would indeed be necessary.
What we have created are essentially two different worlds, one for PCI
devices and one for the rest.
This was indeed not the intention, but it's a fact that basically all
DMA-buf based PCI drivers assume coherent access.
dma-buf does not require universal snooping.
It does defacto require that all device access is coherent with all other
device access, and consistent with the exporters notion of how cpu
coherency is achieved. Not that coherent does not mean snooping, as long
as all devices do unsnooped access and the exporter either does wc/uc or
flushes caches that's perfectly fine, and how all the arm soc dma-buf
sharing works.
We should probably start documenting that better.
We did originally have the wording in there that you have to map/unamp
around every device access, but that got dropped because no one was doing
that anyway.
Now where this totally breaks down is how we make this work, because the
idea was that dma_buf_attach validates this all. Where this means all the
hilarious reasons buffer sharing might not work:
- wrong coherency mode (cpu cached or not)
- not contiguous (we do check that, but only once we get the sg from
dma_buf_attachment_map, which strictly speaking is a bit too late but
most drivers do attach&map as one step so not that bad in practice)
- whether the dma api will throw in bounce buffers or not
- random shit like "oh this is in the wrong memory bank", which I think
never landed in upstream
p2p connectivity is about the only one that gets this right, yay. And the
only reason we can even get it right is because all the information is
exposed to drivers fully.
Yeah, that's why I designed P2P that way :)
I also don't think it's that bad, at least for radeon, nouveau and
amdgpu all the migration restrictions are actually handled correctly.
In other words when a DMA-buf is about to be used by another device we
use TTM to move the buffer around so that it can actually be accessed by
that device.
What I haven't foreseen in here is that we need to deal with different
caching behaviors between exporter and importer.
The issue is that the device dma api refuses to share this information
because it would "leak". Which sucks, because we have defacto build every
single cross-device use-case of dma-buf on the assumption we can check
this (up to gl/vk specs), but oh well.
So in practice this gets sorted out by endless piles of hacks to make
individual use-cases work.
Oh and: This is definitely not limited to arm socs. x86 socs with intel
at least have exactly all the same issues, and they get solved by adding
various shitty hacks to the involved drivers (like i915+amdgpu). Luckily
the intel camera driver isn't in upstream yet, since that would break a
bunch of the hacks since suddently there will be now 2 cpu cache
incoherent devices in an x86 system.
Ideally someone fixes this, but I'm not hopeful.
I recommend pouring more drinks.
What is definitely not correct is claiming that dma-buf wasn't meant for
this. We discussed cache coherency issues endless in budapest 12 or so
years ago, I was there. It's just that the reality of the current
implementation is falling short, and every time someone tries to fix it we
get shouted down by dma api maintainers for looking behind their current.
Well that explains this, I've joined the party a year later and haven't
witnessed all of this.
tldr; You have to magically know to not use cpu cached allocators on these
machines.
Or reject the attachment. As far as I can see that is still the cleanest
option.
Regards,
Christian.
Aside: This is also why vgem alloates wc memory on x86. It's the least
common denominator that works. arm unfortunately doesn't allow you to
allocate wc memory, so there stuff is simply somewhat broken.
-Daniel
