[Intel-gfx] [PATCH 1/6] dma-buf: add dynamic DMA-buf handling v12

[Daniel Vetter]

On Wed, Jun 26, 2019 at 1:53 PM Christian König
<ckoenig.leichtzumerken at gmail.com> wrote:
>
> [SNIP]
> >>>>> I'm confused here: Atm ->moving isn't in resv_obj, there's only one
> >>> exclusive fence. And yes you need to set that every time you do a move
> >>> (because a move needs to be pretty exclusive access). But I'm not seeing a
> >>> separate not_quite_exclusive fence slot for moves.
> >> Yeah, but shouldn't that be sufficient? I mean why does somebody else
> >> than the exporter needs to know when a BO is moving?
> > I think for buffer sharing there's not much use for this, but it
> > sounded like you want to use ->move_notify also more internally. And
> > in that case, for vk, you want to be able to ignore the implicit
> > fences as much as possible. But you can't ignore the buffer moves ofc.
> > Hence tracking those separate could be useful.
>
> Yeah, but for this case I can still rely on using ttm_bo->moving. So no
> need to actually change that.

Hm maybe wasn't clear what I had in mind. Roughly this:

diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 435d02f719a8..33bb4eabe2eb 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -214,8 +214,6 @@ struct ttm_buffer_object {
         * Members protected by a bo reservation.
         */

-       struct dma_fence *moving;
-
        struct drm_vma_offset_node vma_node;

        unsigned priority;
diff --git a/include/linux/reservation.h b/include/linux/reservation.h
index 644a22dbe53b..1c8a8bd077a2 100644
--- a/include/linux/reservation.h
+++ b/include/linux/reservation.h
@@ -74,6 +74,8 @@ struct reservation_object {
        seqcount_t seq;

        struct dma_fence __rcu *fence_excl;
+
+       struct dma_fence _rcu *moving;
        struct reservation_object_list __rcu *fence;
 };

Plus all the sorted fiddling. With the idea that we'd extract a bunch
of these over. Essentially instead of ttm, amdgpu and i915 all having
different ways to track the bo move fences, standardize on one.

> > amdgpu seems to be solving this internally by never attaching an
> > exclusive fence for implicit stuff, or something like that, except
> > when it's shared. But in general you need to assume a funky mix of
> > implicit and explicit sync'ed workloads, and for those tracking the
> > moves separately would be good.
>
> Actually we have an "owner" for each fence which is basically a "void*"
> pointer.
>
> If we see that a command submission is coming from the same "owner" we
> just avoid synchronization at all.
>
> For buffer moves the owner is simply NULL (or some other special value),
> and so we always sync to those.

Hm I guess I'll wait for you to untangel that then.
-Daniel

> [SNIP]
> >>>>> - You sound like you want to use this a lot more, even internally in
> >>>>>      amdgpu. For that I do think the sepearate dma_fence just to make sure
> >>>>>      the buffer is accessible will be needed in resv_obj.
> >>>>>
> >>>>> - Once we have ->moving I think there's some good chances to extract a bit
> >>>>>      of the eviction/pipeline bo move boilerplate from ttm, and maybe use it
> >>>>>      in other drivers. i915 could already make use of this in upstream, since
> >>>>>      we already pipeline get_pages and clflush of buffers. Ofc once we have
> >>>>>      vram support, even more useful.
> >>>> I actually indeed wanted to add more stuff to the reservation object
> >>>> implementation, like finally cleaning up the distinction of readers/writers.
> >>> Hm, more details? Not ringing a bell ...
> >> I'm not yet sure about the details either, so please just wait until I
> >> solved that all up for me first.
> > Ah is this about amdgpu doing something else for implicit sync than
> > what's supposed to be done, and a bit a mismatch when you deal with
> > shared buffers?
>
> Yes, exactly.
>
> >>>> And cleaning up the fence removal hack we have in the KFD for freed up BOs.
> >>>> That would also allow for getting rid of this in the long term.
> >>> Hm, what's that for?
> >> When the KFD frees up memory it removes their eviction fence from the
> >> reservation object instead of setting it as signaled and adding a new
> >> one to all other used reservation objects.
> > Oh so just a fast-path for destryoing memory that's in-flight in some move?
>
> Yes exactly that again.
>
> Christian.



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch