locking&resource refcounting for ttm_bo_kmap/dma_buf_vmap

[Thomas Hellstrom]

On 11/20/19 1:24 PM, Christian König 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?
>
>> 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.

Perhaps with dynamic dma-bufs it might be possible to do something
similar to vmwgfx (and recently other?) fbdev:

The map is cached, but may be invalidated as soon as we release dma_resv
/ unpin. (move_notify() unmaps if needed).

So each time it's needed we make sure we're locked / pinned and then
call a map_validate() function. Typically the map is still around. If it
isn't, the map_validate() function sets it up again.

Saves a bunch of vmap() calls or the need for persistent pinning for
performance reasons.

/Thomas




>
> Christian.
>