[PATCH 2/2] dma-buf: cleanup shared fence removal

[Daniel Vetter]

On Fri, Jun 28, 2019 at 8:32 AM Koenig, Christian
<Christian.Koenig at amd.com> wrote:
> Am 27.06.19 um 21:57 schrieb Daniel Vetter:
> > [SNIP]
> >> Again, the reason to remove the fence from one reservation object is
> >> simply that it is faster to remove it from one object than to attach a
> >> new fence to all other objects.
> > Hm I guess I was lead astray by the eviction_fence invalidation thing
> > in enable_signaling, and a few other places that freed the bo right
> > afters (like amdgpu_amdkfd_gpuvm_free_memory_of_gpu), where removing
> > the fences first and then freeing the bo is kinda pointless.
>
> AH! Now I know where your missing puzzle piece is.
>
> See when we free a buffer object TTM puts it on the delayed delete list
> to make sure that it gets freed up only after all fences are signaled.
>
> So this is essentially to make sure the BO gets freed up immediately.

Well yeah you have to wait for outstanding rendering. Everyone does
that. The problem is that ->eviction_fence does not represent
rendering, it's a very contrived way to implement the ->notify_move
callback.

For real fences you have to wait for rendering to complete, putting
aside corner cases like destroying an entire context with all its
pending rendering. Not really something we should optimize for I
think.

> > Now with your insistence that I'm getting something wrong I guess the
> > you're talking about the unbind case, where the bo survives, but it's
> > mapping disappears, and hence that specific eviction_fence needs to be
> > removed.
> > And yeah there I guess just removing the magic eviction fence is
> > cheaper than replacing all the others.
>
> If possible I actually want to apply this to the general case of freeing
> up per process resources.
>
> In other words when we don't track resource usage on a per submission
> basis freeing up resources is costly because we always have to wait for
> the last submission.
>
> But if we can prevent further access to the resource using page tables
> it is perfectly valid to free it as soon as the page tables are up to date.

Still not seeing how you can use this outside of the magic amdkfd
eviction_fence.

So with the magic amdkfd_eviction fence I agree this makes sense. The
problem I'm having here is that the magic eviction fence itself
doesn't make sense. What I expect will happen (in terms of the new
dynamic dma-buf stuff, I don't have the ttm-ism ready to explain it in
those concepts).

- when you submit command buffers, you _dont_ attach fences to all
involved buffers
- when you get a ->notify_move there's currently 2 options:
1. inefficient way: wait for the latest command buffer to complete, as
a defensive move. To do that you attach the fence from that command
buffer to the obj in your notifiy_move callback (the kerneldoc doesn't
explain this, but I think we really need this).
2. efficient way: You just unmap from pagetables (and eat/handle the
fault if there is any).

No where do you need to remove a fence, because you never attached a
bogus fence.

Now with the magic eviction trick amdkfd uses, you can't do that,
because you need to attach this magic fence to all buffers, all the
time. And since it still needs to work like a fence it's one-shot
only, i.e. instead of a reuseable ->notify_move callback you need to
create a new fence every time ->enable_signalling is called. So in a
way replacing fences is just an artifact of some very, very crazy
calling convention.

If you have a real callback, there's no need for cycling through
fences, and therefore there's also no need to optimize their removal.

Or did you work under the assumption that ->notify_move cannot attach
new fences, and therefore you'd have to roll this magic fence trick to
even more places?

> > Now I guess I understand the mechanics of this somewhat, and what
> > you're doing, and lit ooks even somewhat safe. But I have no idea what
> > this is supposed to achieve. It feels a bit like ->notify_move, but
> > implemented in the most horrible way possible. Or maybe something
> > else.
> >
> > Really no idea.
> >
> > And given that we've wasted I few pages full of paragraphs already on
> > trying to explain what your new little helper is for, when it's safe
> > to use, when it's maybe not a good idea, and we still haven't even
> > bottomed out on what this is for ... well I really don't think it's a
> > good idea to inflict this into core code. Because just blindly
> > removing normal fences is not safe.
> >
> > Especially with like half a sentence of kerneldoc that explains
> > nothing of all this complexity.

Still makes no sense to me to have in core code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch