locking&resource refcounting for ttm_bo_kmap/dma_buf_vmap

[Daniel Vetter]

On Wed, Nov 20, 2019 at 1:24 PM Christian König
<christian.koenig at amd.com> wrote:
>
> Am 20.11.19 um 13:19 schrieb Daniel Vetter:
> > On Wed, Nov 20, 2019 at 1:09 PM Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> >> On Wed, Nov 20, 2019 at 1:02 PM Christian König
> >> <christian.koenig at amd.com> wrote:
> >>>> What am I missing?
> >>> The assumption is that when you want to create a vmap of a DMA-buf
> >>> buffer the buffer needs to be pinned somehow.
> >>>
> >>> E.g. without dynamic dma-buf handling you would need to have an active
> >>> attachment. With dynamic handling the requirements could be lowered to
> >>> using the pin()/unpin() callbacks.
> >> Yeah right now everyone seems to have an attachment, and that's how we
> >> get away with all this. But the interface isn't supposed to work like
> >> that, dma_buf_vmap/unmap is supposed to be a stand-alone thing (you
> >> can call it directly on the struct dma_buf, no need for an
> >> attachment). Also I don't think non-dynamic drivers should ever call
> >> pin/unpin, not their job, holding onto a mapping should be enough to
> >> get things pinned.
> >>
> >>> You also can't lock/unlock inside your vmap callback because you don't
> >>> have any guarantee that the pointer stays valid as soon as your drop
> >>> your lock.
> >> Well that's why I asked where the pin/unpin pair is. If you lock&pin,
> >> then you do know that the pointer will stay around. But looks like the
> >> original patch from Dave for ttm based drivers played fast&loose here
> >> with what should be done.
> >>
> >>> BTW: What is vmap() still used for?
> >> udl, bunch of other things (e.g. bunch of tiny drivers). Not much, but
> >> not stuff we can drop.
> > If we're unlucky we'll actually have a problem with these now. For
> > some of these the attached device is not dma-capable, so dma_map_sg
> > will go boom. With the cached mapping logic we now have this might go
> > sideways for dynamic exporters. Did you test your dynamic dma-buf
> > support for amdgpu with udl?
>
> Short answer no, not at all. Long one: What the heck is udl? And how is
> it not dma-capable?

usb display thing. The data gets shoveled over the wire with cpu
vmaps. A bunch of the tiny drivers we have have similar issues, where
the controller might fall back to PIO instead of dma. Either way the
struct device itself is not sitting on a bus that can do dma directly
- I guess the devices could (maybe they do already, didn't check that)
attach to the bus master, but sometimes that's fairly opaque and
hidden in the subsystem plumbing.

Entire different case is fully fake stuff like vgem/vkms. If that
still works we should be good (at least on x86, dma-api works
differently on other platforms). And again you'd need to test with
your dynamic dma-buf exporter to verify, otherwise I don't think we'll
hit the critical path.

> > Worst case we need to get rid of the fake
> > attachment, fix the vmap locking/pinning, and maybe some more
> > headaches to sort this out.
>
> Well of hand we could require that vmap will also pin a DMA-buf and
> start fixing amgpu/nouveau/radeon/qxl.

Yeah that's what I meant with lock/pin fixing. But it's only one side,
we'd also need to get rid of the dma_map_sg for all these drivers that
don't dma (whether virtual or on a bus without dma).
-Daniel

>
> Christian.
>
> > -Daniel
> >
> >
> >> -Daniel
> >>
> >>> Regards,
> >>> Christian.
> >>>
> >>> Am 20.11.19 um 12:47 schrieb Daniel Vetter:
> >>>> Hi all,
> >>>>
> >>>> I've been looking at dma_buf_v(un)map, with a goal to standardize
> >>>> locking for at least dynamic dma-buf exporters/importers, most likely
> >>>> by requiring dma_resv_lock. And I got questions around how this is
> >>>> supposed to work, since a big chunk of drivers seem to entirely lack
> >>>> locking around ttm_bo_kmap. Two big ones:
> >>>>
> >>>> - ttm_bo_kmap looks at bo->mem to figure out what/where to kmap to get
> >>>> at that buffer. bo->mem is supposed to be protected with
> >>>> dma_resv_lock, but at least amgpu/nouveau/radeon/qxl don't grab that
> >>>> in their prime vmap function.
> >>>>
> >>>> - between the vmap and vunmap something needs to make sure the backing
> >>>> storage doesn't move around. I didn't find that either anywhere,
> >>>> ttm_bo_kmap simply seems to set up the mapping, leaving locking and
> >>>> refcounting to callers.
> >>>>
> >>>> - vram helpers have at least locking, but I'm still missing the
> >>>> refcounting. vmwgfx doesn't bother with vmap.
> >>>>
> >>>> What am I missing?
> >>>>
> >>>> Thanks, Daniel
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> >
>


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