locking&resource refcounting for ttm_bo_kmap/dma_buf_vmap

[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? 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.
-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