mbox series

[v1,0/7] Move dma-buf mmap() reservation locking down to exporters

Message ID 20230402164826.752842-1-dmitry.osipenko@collabora.com
Headers
Series Move dma-buf mmap() reservation locking down to exporters |

Message

Dmitry Osipenko April 2, 2023, 4:48 p.m. UTC 
  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@sync@rcs0.html
[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(-)

Comments

Dmitry Osipenko April 3, 2023, 10:19 a.m. UTC | #1 
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@sync@rcs0.html
> [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/