Am 19.05.21 um 00:06 schrieb Jason Ekstrand:
[SNIP]
E.g. we can't add a fence which doesn't wait for the exclusive one as
shared.
Ok I think that's a real problem, and guess it's also related to all
the ttm privatization tricks and all that. So essentially we'd need
the opposite of ttm_bo->moving, as in you can't ignore it, but
otherwise it completely ignores all the userspace implicit fence
stuff.
Would you mind explaining it to the rest of the class? I get the need
to do a TLB flush after a BO is removed from the processes address
space and I get that it may be super-heavy and that it has to be
delayed. I also get that the driver needs to hold a reference to the
underlying pages until that TLB flush is done. What I don't get is
what this has to do with the exclusive fence. Why can't the driver
just gather up all the dma_resv fences on the current object (or,
better yet, just the ones from the current amdgpu process) and wait on
them all? Why does it need to insert an exclusive fence that then
clogs up the whole works?
Because we have mixed up resource management with implicit syncing.
When I sum up all fences in (for example) a dma_fence_array container
and add that as explicit fence to the dma_resv object resource
management will do what I want and wait for everything to finish before
moving or freeing the buffer. But implicit sync will just horrible over
sync and wait for stuff it shouldn't wait for in the first place.
When I add the fence as shared fence I can run into the problem the the
TLB flush might finish before the exclusive fence. Which is not allowed
according to the DMA-buf fencing rules.
We currently have some rather crude workarounds to make use cases like
this work as expected. E.g. by using a
dma_fence_chain()/dma_fence_array() and/or adding the explusive fence to
the shared fences etc etc...
Let's say that you have a buffer which is shared between two drivers A
and B and let's say driver A has thrown a fence on it just to ensure
that the BO doesn't get swapped out to disk until it's at a good
stopping point. Then driver B comes along and wants to throw a
write-fence on it. Suddenly, your memory fence from driver A causes
driver B to have to stall waiting for a "good" time to throw in a
fence. It sounds like this is the sort of scenario that Christian is
running into. And, yes, with certain Vulkan drivers being a bit
sloppy about exactly when they throw in write fences, I could see it
being a real problem.
Yes this is a potential problem, and on the i915 side we need to do
some shuffling here most likely. Especially due to discrete, but the
problem is pre-existing. tbh I forgot about the implications here
until I pondered this again yesterday evening.
But afaiui the amdgpu code and winsys in mesa, this isn't (yet) the
problem amd vk drivers have. The issue is that with amdgpu, all you
supply are the following bits at CS time:
- list of always mapped private buffers, which is implicit and O(1) in
the kernel fastpath
- additional list of shared buffers that are used by the current CS
I didn't check how exactly that works wrt winsys buffer ownership, but
the thing is that on the kernel side _any_ buffer in there is treated
as a implicit sync'ed write. Which means if you render your winsys
with a bunch of command submission split over 3d and compute pipes,
you end up with horrendous amounts of oversync.
What are you talking about? We have no sync at all for submissions from
the same client.
Yes. Except when the buffer is shared with another driver, at which
point you sync a _lot_ and feel the pain.
Yes, exactly that's the problem.
We basically don't know during CS if a BO is shared or not.
We do know that during importing or exporting the BO thought.
No you don't. Or at least that's massively awkward, see Jason's reply.
Please. In Vulkan, we know explicitly whether or not any BO will ever
be shared and, if a BO is ever flagged as shared even though it's not,
that's the app being stupid and they can eat the perf hit.
Yeah, that's not a problem at all. We already have the per BO flag in
amdgpu for this as well.
In GL, things are more wishy-washy but GL has so many stupid cases where we
have to throw a buffer away and re-allocate that one more isn't going
to be all that bad. Even there, you could do something where you add
an in-fence to the BO export operation so that the driver knows when
to switch from the shared internal dma_resv to the external one
without having to create a new BO and copy.
Hui what? What do you mean with in-fence here?
[SNIP]
Yeah but why does your userspace not know when a bo is used?
We always know when a BO is exported because we're the ones doing the
export call. Always. Of course, we don't know if that BO is shared
with another driver or re-imported back into the same one but is that
really the case we're optimizing for?
Yes, unfortunately. Exactly that's one of the reasons we couldn't go
with the per CS per BO flag if it should be shared or exclusive.
Or very bluntly, why cant radv do what anv does (or amdvlk if you care
more about that, it's the same)? What's missing with lots of blantant
lying?
I'm also not buying this. You keep claiming that userspace doesn't
know but GL definitely does know and Vulkan knows well enough. You
say that it's motivated by Vulkan and use RADV as an example but the
only reason why the RADV guys haven't followed the ANV design is to
work around limitations in amdgpu. We shouldn't then use RADV to
justify why this is the right uAPI and why i915 is wrong.
Well, I never said that this is because of RADV. The main motivation we
had is because of MM engines, e.g. VA-API, VDPAU and OpenMax.
And when we expose a BO with the DMA-buf functions we simply doesn't
know in userspace if that is then re-imported into VA-API or send to a
different process.
[SNIP]
Yeah, and that is exactly the reason why I will NAK this uAPI change.
This doesn't works for amdgpu at all for the reasons outlined above.
Uh that's really not how uapi works. "my driver is right, everyone
else is wrong" is not how cross driver contracts are defined. If that
means a perf impact until you've fixed your rules, that's on you.
Also you're a few years too late with nacking this, it's already uapi
in the form of the dma-buf poll() support.
^^ My fancy new ioctl doesn't expose anything that isn't already
there. It just lets you take a snap-shot of a wait instead of doing
an active wait which might end up with more fences added depending on
interrupts and retries. The dma-buf poll waits on all fences for
POLLOUT and only the exclusive fence for POLLIN. It's already uAPI.
Well that's not the stuff I'm concerned about. But rather that you want
to add that as exclusive fence from the shared ones once more.
This prevents the TLB flush case I've outlined from working correctly.
So the way I see things right now:
- exclusive fence slot is for implicit sync. kmd should only set it
when userspace indicates, otherwise you will suffer. Explicit syncing
userspace needs to tell the kernel with a flag in the CS ioctl when it
should sync against this exclusive fence and when it should ignore it,
otherwise you'll suffer badly once more.
That is not sufficient. The explicit sync slot is for kernel internal
memory management.
Then we need to split it. But what I discussed with Thomas Hellstrom
is that at least for anything except p2p dma-buf ttm_bo->moving should
be enough.
This is starting to sound like maybe roughly the right direction to me
but I'm still unclear on exactly what problem we're trying to solve
for TLB invalidates. I'd like to understand that better before giving
strong opinions. I'm also not super-familiar with ttm_bo->moving but
it sounds like we need some third category of fence somewhere.
Well I would rather say that we should separate the use cases.
E.g. clear APIs for resource management vs. implicit sync.
Christian.
--Jason
