This patchset makes dma-buf exporters responisble for taking care of
the reservation lock. I also included patch that moves drm-shmem to use
reservation lock, to let CI test the whole set. I'm going to take all
the patches via the drm-misc tree, please give an ack.
Previous policy stated that dma-buf core takes the lock around mmap()
callback. Which meant that both importers and exporters shouldn't touch
the reservation lock in the mmap() code path. This worked well until
Intel-CI found a deadlock problem in a case of self-imported dma-buf [1].
The problem happens when userpace mmaps a self-imported dma-buf, i.e.
mmaps the dma-buf FD. DRM core treats self-imported dma-bufs as own GEMs
[2]. There is no way to differentiate a prime GEM from a normal GEM for
drm-shmem in drm_gem_shmem_mmap(), which resulted in a deadlock problem
for drm-shmem mmap() code path once it's switched to use reservation lock.
It was difficult to fix the drm-shmem problem without adjusting dma-buf
locking policy. In parctice not much changed from importers perspective
because previosly dma-buf was taking the lock in between of importers
and exporters. Now this lock is shifted down to exporters.
[1] https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114671v2/shard-snb5/igt@prime_vgem@[email protected]
[2] https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/drm_prime.c#L924
Dmitry Osipenko (7):
Revert "media: videobuf2: Assert held reservation lock for dma-buf
mmapping"
Revert "dma-buf/heaps: Assert held reservation lock for dma-buf
mmapping"
Revert "udmabuf: Assert held reservation lock for dma-buf mmapping"
Revert "fastrpc: Assert held reservation lock for dma-buf mmapping"
Revert "drm: Assert held reservation lock for dma-buf mmapping"
dma-buf: Change locking policy for mmap()
drm/shmem-helper: Switch to reservation lock
drivers/dma-buf/dma-buf.c | 17 +-
drivers/dma-buf/heaps/cma_heap.c | 3 -
drivers/dma-buf/heaps/system_heap.c | 3 -
drivers/dma-buf/udmabuf.c | 2 -
drivers/gpu/drm/drm_gem_shmem_helper.c | 217 ++++++++----------
drivers/gpu/drm/drm_prime.c | 2 -
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 -
drivers/gpu/drm/lima/lima_gem.c | 8 +-
drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 2 -
drivers/gpu/drm/panfrost/panfrost_drv.c | 7 +-
.../gpu/drm/panfrost/panfrost_gem_shrinker.c | 6 +-
drivers/gpu/drm/panfrost/panfrost_mmu.c | 19 +-
drivers/gpu/drm/tegra/gem.c | 2 -
.../common/videobuf2/videobuf2-dma-contig.c | 3 -
.../media/common/videobuf2/videobuf2-dma-sg.c | 3 -
.../common/videobuf2/videobuf2-vmalloc.c | 3 -
drivers/misc/fastrpc.c | 3 -
include/drm/drm_gem_shmem_helper.h | 14 +-
18 files changed, 123 insertions(+), 193 deletions(-)
--
2.39.2
Change locking policy of mmap() callback, making exporters responsible
for handling dma-buf reservation locking. Previous locking policy stated
that dma-buf is locked for both importers and exporters by the dma-buf
core, which caused a deadlock problem for DRM drivers in a case of
self-imported dma-bufs which required to take the lock from the DRM
exporter side.
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/dma-buf/dma-buf.c | 17 +++--------------
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index aa4ea8530cb3..21916bba77d5 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -131,7 +131,6 @@ static struct file_system_type dma_buf_fs_type = {
static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma)
{
struct dma_buf *dmabuf;
- int ret;
if (!is_dma_buf_file(file))
return -EINVAL;
@@ -147,11 +146,7 @@ static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma)
dmabuf->size >> PAGE_SHIFT)
return -EINVAL;
- dma_resv_lock(dmabuf->resv, NULL);
- ret = dmabuf->ops->mmap(dmabuf, vma);
- dma_resv_unlock(dmabuf->resv);
-
- return ret;
+ return dmabuf->ops->mmap(dmabuf, vma);
}
static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence)
@@ -850,6 +845,7 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
* - &dma_buf_ops.release()
* - &dma_buf_ops.begin_cpu_access()
* - &dma_buf_ops.end_cpu_access()
+ * - &dma_buf_ops.mmap()
*
* 2. These &dma_buf_ops callbacks are invoked with locked dma-buf
* reservation and exporter can't take the lock:
@@ -858,7 +854,6 @@ static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
* - &dma_buf_ops.unpin()
* - &dma_buf_ops.map_dma_buf()
* - &dma_buf_ops.unmap_dma_buf()
- * - &dma_buf_ops.mmap()
* - &dma_buf_ops.vmap()
* - &dma_buf_ops.vunmap()
*
@@ -1463,8 +1458,6 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_end_cpu_access, DMA_BUF);
int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
unsigned long pgoff)
{
- int ret;
-
if (WARN_ON(!dmabuf || !vma))
return -EINVAL;
@@ -1485,11 +1478,7 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
vma_set_file(vma, dmabuf->file);
vma->vm_pgoff = pgoff;
- dma_resv_lock(dmabuf->resv, NULL);
- ret = dmabuf->ops->mmap(dmabuf, vma);
- dma_resv_unlock(dmabuf->resv);
-
- return ret;
+ return dmabuf->ops->mmap(dmabuf, vma);
}
EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, DMA_BUF);
--
2.39.2
On 4/2/23 19:48, Dmitry Osipenko wrote:
> This patchset makes dma-buf exporters responisble for taking care of
> the reservation lock. I also included patch that moves drm-shmem to use
> reservation lock, to let CI test the whole set. I'm going to take all
> the patches via the drm-misc tree, please give an ack.
>
> Previous policy stated that dma-buf core takes the lock around mmap()
> callback. Which meant that both importers and exporters shouldn't touch
> the reservation lock in the mmap() code path. This worked well until
> Intel-CI found a deadlock problem in a case of self-imported dma-buf [1].
>
> The problem happens when userpace mmaps a self-imported dma-buf, i.e.
> mmaps the dma-buf FD. DRM core treats self-imported dma-bufs as own GEMs
> [2]. There is no way to differentiate a prime GEM from a normal GEM for
> drm-shmem in drm_gem_shmem_mmap(), which resulted in a deadlock problem
> for drm-shmem mmap() code path once it's switched to use reservation lock.
>
> It was difficult to fix the drm-shmem problem without adjusting dma-buf
> locking policy. In parctice not much changed from importers perspective
> because previosly dma-buf was taking the lock in between of importers
> and exporters. Now this lock is shifted down to exporters.
>
> [1] https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114671v2/shard-snb5/igt@prime_vgem@[email protected]
> [2] https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/drm_prime.c#L924
>
> Dmitry Osipenko (7):
> Revert "media: videobuf2: Assert held reservation lock for dma-buf
> mmapping"
> Revert "dma-buf/heaps: Assert held reservation lock for dma-buf
> mmapping"
> Revert "udmabuf: Assert held reservation lock for dma-buf mmapping"
> Revert "fastrpc: Assert held reservation lock for dma-buf mmapping"
> Revert "drm: Assert held reservation lock for dma-buf mmapping"
> dma-buf: Change locking policy for mmap()
> drm/shmem-helper: Switch to reservation lock
>
> drivers/dma-buf/dma-buf.c | 17 +-
> drivers/dma-buf/heaps/cma_heap.c | 3 -
> drivers/dma-buf/heaps/system_heap.c | 3 -
> drivers/dma-buf/udmabuf.c | 2 -
> drivers/gpu/drm/drm_gem_shmem_helper.c | 217 ++++++++----------
> drivers/gpu/drm/drm_prime.c | 2 -
> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 -
> drivers/gpu/drm/lima/lima_gem.c | 8 +-
> drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 2 -
> drivers/gpu/drm/panfrost/panfrost_drv.c | 7 +-
> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 6 +-
> drivers/gpu/drm/panfrost/panfrost_mmu.c | 19 +-
> drivers/gpu/drm/tegra/gem.c | 2 -
> .../common/videobuf2/videobuf2-dma-contig.c | 3 -
> .../media/common/videobuf2/videobuf2-dma-sg.c | 3 -
> .../common/videobuf2/videobuf2-vmalloc.c | 3 -
> drivers/misc/fastrpc.c | 3 -
> include/drm/drm_gem_shmem_helper.h | 14 +-
> 18 files changed, 123 insertions(+), 193 deletions(-)
>
Intel's IGT passed[1] (see Tests section) with the new locking policy,
which failed previously for the "drm/shmem-helper: Switch to reservation
lock" patch.
[1] https://patchwork.freedesktop.org/series/116000/
--
Best regards,
Dmitry