Re: [PATCH v4 10/15] drm/shmem-helper: Take reservation lock instead of drm_gem_shmem locks

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

On 5/4/22 11:21, Daniel Vetter wrote:
...
>>> - Maybe also do what you suggest and keep a separate lock for this, but
>>>   the fundamental issue is that this doesn't really work - if you share
>>>   buffers both ways with two drivers using shmem helpers, then the
>>>   ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and
>>>   you can get some nice deadlocks. So not a great approach (and also the
>>>   reason why we really need to get everyone to move towards dma_resv_lock
>>>   as _the_ buffer object lock, since otherwise we'll never get a
>>>   consistent lock nesting hierarchy).
>>
>> The separate locks should work okay because it will be always the
>> exporter that takes the dma_resv_lock. But I agree that it's less ideal
>> than defining the new rules for dma-bufs since sometime you will take
>> the resv lock and sometime not, potentially hiding bugs related to lockings.
> 
> That's the issue, some importers need to take the dma_resv_lock for
> dma_buf_vmap too (e.g. to first nail the buffer in place when it's a
> dynamic memory manager). In practice it'll work as well as what we have
> currently, which is similarly inconsistent, except with per-driver locks
> instead of shared locks from shmem helpers or dma-buf, so less obvious
> that things are inconsistent.
> 
> So yeah if it's too messy maybe the approach is to have a separate lock
> for vmap for now, land things, and then fix up dma_buf_vmap in a follow up
> series.

The amdgpu driver was the fist who introduced the concept of movable
memory for dma-bufs. Now we want to support it for DRM SHMEM too. For
both amdgpu ttm and shmem drivers we will want to hold the reservation
lock when we're touching moveable buffers. The current way of denoting
that dma-buf is movable is to implement the pin/unpin callbacks of the
dma-buf ops, should be doable for shmem.

A day ago I found that mapping of imported dma-bufs is broken at least
for the Tegra DRM driver (and likely for others too) because driver
doesn't assume that anyone will try to mmap imported buffer and just
doesn't handle this case at all, so we're getting a hard lockup on
touching mapped memory because we're mapping something else than the
dma-buf.

My plan is to move the dma-buf management code to the level of DRM core
and make it aware of the reservation locks for the dynamic dma-bufs.
This way we will get the proper locking for dma-bufs and fix mapping of
imported dma-bufs for Tegra and other drivers.

-- 
Best regards,
Dmitry